Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix Metadata Warning Locations #3014

Merged
merged 4 commits into from
Oct 31, 2024
+114 −118
Merged

Fix Metadata Warning Locations #3014

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4812e38
Use metadata location tracking when reporting warnings.
InsertCreativityHere Oct 30, 2024
65bc834
Updated metadata tests to match new line numbers.
InsertCreativityHere Oct 30, 2024
e03ffb1
Review comments and formatting.
InsertCreativityHere Oct 31, 2024
aafb3ab
Bad copy/paste from adding parameter names...
InsertCreativityHere Oct 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cpp/src/Slice/Scanner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1721,7 +1721,7 @@ case 16:
YY_RULE_SETUP
#line 233 "src/Slice/Scanner.l"
{
currentUnit->warning(All, "unknown escape sequence in string literal: `" + string(yytext) + "'");
currentUnit->warning(All, "unknown escape sequence in string literal: '" + string{yytext} + "'");

StringTokPtr str = dynamic_pointer_cast<StringTok>(*yylval);
// Escape the entire sequence.
Expand Down
2 changes: 1 addition & 1 deletion cpp/src/Slice/Scanner.l
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ floating_literal (({fractional_constant}{exponent_part}?)|((\+|-)?{dec}+{expo
/* Matches an unknown escape value. This rule has a lower priority than all the other escape rules because
* it only matches 2 characters (the lowest any match), and it's beneath the others. */
<STRING_LITERAL>"\\". {
currentUnit->warning(All, "unknown escape sequence in string literal: `" + string(yytext) + "'");
currentUnit->warning(All, "unknown escape sequence in string literal: '" + string{yytext} + "'");

StringTokPtr str = dynamic_pointer_cast<StringTok>(*yylval);
// Escape the entire sequence.
Expand Down
43 changes: 21 additions & 22 deletions cpp/src/slice2cpp/Gen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -786,7 +786,7 @@ Slice::Gen::generate(const UnitPtr& p)
{
ostringstream ostr;
ostr << "ignoring invalid file metadata '" << *metadata << "'";
dc->warning(InvalidMetadata, file, -1, ostr.str());
dc->warning(InvalidMetadata, metadata->file(), metadata->line(), ostr.str());
fileMetadata.remove(metadata);
}
}
Expand All @@ -800,7 +800,7 @@ Slice::Gen::generate(const UnitPtr& p)
{
ostringstream ostr;
ostr << "ignoring invalid file metadata '" << *metadata << "'";
dc->warning(InvalidMetadata, file, -1, ostr.str());
dc->warning(InvalidMetadata, metadata->file(), metadata->line(), ostr.str());
fileMetadata.remove(metadata);
}
}
Expand Down Expand Up @@ -947,7 +947,7 @@ Slice::Gen::MetadataVisitor::visitUnitStart(const UnitPtr& unit)
ostringstream ostr;
ostr << "ignoring invalid file metadata '" << *s
<< "': directive can appear only once per file";
dc->warning(InvalidMetadata, file, -1, ostr.str());
dc->warning(InvalidMetadata, s->file(), s->line(), ostr.str());
fileMetadata.remove(s);
}
seenHeaderExtension = true;
Expand All @@ -960,7 +960,7 @@ Slice::Gen::MetadataVisitor::visitUnitStart(const UnitPtr& unit)
ostringstream ostr;
ostr << "ignoring invalid file metadata '" << *s
<< "': directive can appear only once per file";
dc->warning(InvalidMetadata, file, -1, ostr.str());
dc->warning(InvalidMetadata, s->file(), s->line(), ostr.str());
fileMetadata.remove(s);
}
seenSourceExtension = true;
Expand All @@ -973,7 +973,7 @@ Slice::Gen::MetadataVisitor::visitUnitStart(const UnitPtr& unit)
ostringstream ostr;
ostr << "ignoring invalid file metadata '" << *s
<< "': directive can appear only once per file";
dc->warning(InvalidMetadata, file, -1, ostr.str());
dc->warning(InvalidMetadata, s->file(), s->line(), ostr.str());
fileMetadata.remove(s);
}
seenDllExport = true;
Expand All @@ -986,7 +986,7 @@ Slice::Gen::MetadataVisitor::visitUnitStart(const UnitPtr& unit)

ostringstream ostr;
ostr << "ignoring invalid file metadata '" << *s << "'";
dc->warning(InvalidMetadata, file, -1, ostr.str());
dc->warning(InvalidMetadata, s->file(), s->line(), ostr.str());
fileMetadata.remove(s);
}
}
Expand All @@ -999,34 +999,34 @@ Slice::Gen::MetadataVisitor::visitUnitStart(const UnitPtr& unit)
bool
Slice::Gen::MetadataVisitor::visitModuleStart(const ModulePtr& p)
{
p->setMetadata(validate(p, p->getMetadata(), p->file(), p->line()));
p->setMetadata(validate(p, p->getMetadata(), p->file()));
return true;
}

void
Slice::Gen::MetadataVisitor::visitClassDecl(const ClassDeclPtr& p)
{
p->setMetadata(validate(p, p->getMetadata(), p->file(), p->line()));
p->setMetadata(validate(p, p->getMetadata(), p->file()));
}

bool
Slice::Gen::MetadataVisitor::visitClassDefStart(const ClassDefPtr& p)
{
p->setMetadata(validate(p, p->getMetadata(), p->file(), p->line()));
p->setMetadata(validate(p, p->getMetadata(), p->file()));
return true;
}

bool
Slice::Gen::MetadataVisitor::visitExceptionStart(const ExceptionPtr& p)
{
p->setMetadata(validate(p, p->getMetadata(), p->file(), p->line()));
p->setMetadata(validate(p, p->getMetadata(), p->file()));
return true;
}

bool
Slice::Gen::MetadataVisitor::visitStructStart(const StructPtr& p)
{
p->setMetadata(validate(p, p->getMetadata(), p->file(), p->line()));
p->setMetadata(validate(p, p->getMetadata(), p->file()));
return true;
}

Expand All @@ -1047,59 +1047,58 @@ Slice::Gen::MetadataVisitor::visitOperation(const OperationPtr& p)
{
ostringstream ostr;
ostr << "ignoring invalid metadata '" << *s << "' for operation with void return type";
dc->warning(InvalidMetadata, p->file(), p->line(), ostr.str());
dc->warning(InvalidMetadata, s->file(), s->line(), ostr.str());
metadata.remove(s);
}
}
p->setMetadata(std::move(metadata));
}
else
{
p->setMetadata(validate(returnType, p->getMetadata(), p->file(), p->line(), true));
p->setMetadata(validate(returnType, p->getMetadata(), p->file(), true));
}

for (const auto& param : p->parameters())
{
param->setMetadata(validate(param->type(), param->getMetadata(), p->file(), param->line(), true));
param->setMetadata(validate(param->type(), param->getMetadata(), p->file(), true));
}
}

void
Slice::Gen::MetadataVisitor::visitDataMember(const DataMemberPtr& p)
{
p->setMetadata(validate(p->type(), p->getMetadata(), p->file(), p->line()));
p->setMetadata(validate(p->type(), p->getMetadata(), p->file()));
}

void
Slice::Gen::MetadataVisitor::visitSequence(const SequencePtr& p)
{
p->setMetadata(validate(p, p->getMetadata(), p->file(), p->line()));
p->setMetadata(validate(p, p->getMetadata(), p->file()));
}

void
Slice::Gen::MetadataVisitor::visitDictionary(const DictionaryPtr& p)
{
p->setMetadata(validate(p, p->getMetadata(), p->file(), p->line()));
p->setMetadata(validate(p, p->getMetadata(), p->file()));
}

void
Slice::Gen::MetadataVisitor::visitEnum(const EnumPtr& p)
{
p->setMetadata(validate(p, p->getMetadata(), p->file(), p->line()));
p->setMetadata(validate(p, p->getMetadata(), p->file()));
}

void
Slice::Gen::MetadataVisitor::visitConst(const ConstPtr& p)
{
p->setMetadata(validate(p, p->getMetadata(), p->file(), p->line()));
p->setMetadata(validate(p, p->getMetadata(), p->file()));
}

MetadataList
Slice::Gen::MetadataVisitor::validate(
const SyntaxTreeBasePtr& cont,
MetadataList metadata,
const string& file,
int line,
bool operation)
{
const UnitPtr ut = cont->unit();
Expand All @@ -1118,7 +1117,7 @@ Slice::Gen::MetadataVisitor::validate(
{
ostringstream ostr;
ostr << "ignoring invalid metadata '" << *meta << "'";
dc->warning(InvalidMetadata, file, line, ostr.str());
dc->warning(InvalidMetadata, meta->file(), meta->line(), ostr.str());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite follow the file vs meta->file() in this function. Are they always the same?
Apparently, file and cont are only used to get dc and dc is only used to issue warnings.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Long story short, it's always safest to get the metadata's file with meta->file().
Most of the time, you're right that it will be the same as file, but there's some edge cases where they can differ:

// File1
[good-metadata] sequence<int> IntSequence;

// File2
struct S
{
    [bad-metadata] IntSequence is;
}

Here, file is the file of the actual sequence definition (File1), but we want to issue the warning for File2.

I think we can eliminate the need to get the dc with a file in the first place, and let warning do the lookup using the file you pass into it. But obviously, we'd need to move warning somewhere else first.
One step at a time though. If you stare at any of our code long enough, you'll find an endless pit of refactoring.

metadata.remove(meta);
continue;
}
Expand Down Expand Up @@ -1184,7 +1183,7 @@ Slice::Gen::MetadataVisitor::validate(

ostringstream ostr;
ostr << "ignoring invalid metadata '" << *meta << "'";
dc->warning(InvalidMetadata, file, line, ostr.str());
dc->warning(InvalidMetadata, meta->file(), meta->line(), ostr.str());
metadata.remove(meta);
}
return metadata;
Expand Down
2 changes: 1 addition & 1 deletion cpp/src/slice2cpp/Gen.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ namespace Slice
void visitConst(const ConstPtr&) final;

private:
MetadataList validate(const SyntaxTreeBasePtr&, MetadataList, const std::string&, int, bool = false);
MetadataList validate(const SyntaxTreeBasePtr&, MetadataList, const std::string&, bool = false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's nice to add param names when updating functions. Especially when the param names are non-obvious.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should also add a comment describing what this visitor does; presumably, validate cpp:... metadata.

};

static void validateMetadata(const UnitPtr&);
Expand Down
4 changes: 2 additions & 2 deletions cpp/src/slice2cs/CsUtil.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1982,7 +1982,7 @@ Slice::CsGenerator::MetadataVisitor::visitUnitStart(const UnitPtr& unit)
{
ostringstream msg;
msg << "ignoring invalid file metadata '" << *metadata << "'";
dc->warning(InvalidMetadata, file, -1, msg.str());
dc->warning(InvalidMetadata, metadata->file(), metadata->line(), msg.str());
continue;
}
newFileMetadata.push_back(metadata);
Expand Down Expand Up @@ -2173,7 +2173,7 @@ Slice::CsGenerator::MetadataVisitor::validate(const ContainedPtr& cont)

ostringstream msg;
msg << "ignoring invalid metadata '" << *metadata << "'";
dc->warning(InvalidMetadata, cont->file(), cont->line(), msg.str());
dc->warning(InvalidMetadata, metadata->file(), metadata->line(), msg.str());
continue;
}
newLocalMetadata.push_back(metadata);
Expand Down
Loading
Loading