Fix Metadata Warning Locations #3014
Conversation
|
I fixed the clang-format error locally, I'm just waiting to let the other tests finish before I push it (or they'd restart). |
cpp/src/slice2cpp/Gen.h
Outdated
| @@ -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); | |||
There was a problem hiding this comment.
It's nice to add param names when updating functions. Especially when the param names are non-obvious.
There was a problem hiding this comment.
You should also add a comment describing what this visitor does; presumably, validate cpp:... metadata.
| @@ -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()); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
This PR fixes #2021.
Before my refactoring of metadata, we didn't store the metadata's location. So when reporting warnings, we either had to omit location information (by passing
-1) of guess that it was the same line as some other Slice definition.Now, we use the metadata's location properly.
You can see this in the test that had to be fixed at the bottom of this PR.