Generalization & minor refactoring in RenameMap #4677
Conversation
vlstill
added
the
core
| auto name = new IR::ID(orig->getName().srcInfo, newName, orig->getName().originalName); | ||
| auto newName = renameMap->get(orig); | ||
| if (!newName.has_value()) return nullptr; | ||
| auto name = new IR::ID(orig->getName().srcInfo, *newName, orig->getName().originalName); |
There was a problem hiding this comment.
Tangential comment but what if originalName is nullptr? Then the name is lost.
Maybe this should call toString?
There was a problem hiding this comment.
Tangential comment but what if
originalNameisnullptr? Then the name is lost.
How would it be lost? It would just make an ID where the name is set and originalName is nullptr, which according to the IR::ID doc is perfectly fine and intended to be used for compiler-generated names (another one of the things I only discover because of checking something else :-/).
There was a problem hiding this comment.
Assuming the IDeclaration has an IR::ID which only has a name, but not yet an original name (because it is the original). Now we create a new IR::ID and pass the newName along but not original name. originalName is never set and the initial name of the declaration is lost.
Although now that you point it out I am not sure whether that is intended or not...
Iirc I definitely have run into a variant of this problem before but never pursued fixing it. #4444
There was a problem hiding this comment.
The two-parameter (source loc + name) and single param (just name) constructor of the IR::ID sets both name and originalName (https://github.com/p4lang/p4c/blob/main/ir/id.h#L37). So the only way to create IR::ID without original name should be explicitly setting it to nullptr (either in constructor, or afterward). So if there was no renaming and the ID originates from parsed code, it will have both names equal. After renaming it here the originalName is preserved.
It is possible there is a place where originalName is not preserved when creating IDs, but I'd say it is elsewhere (maybe in inlining from what you wrote).
There was a problem hiding this comment.
originalName should always be set for IDs that correspond to things in the source code -- originalName == nullptr implies that this is a purely compiler generated symbol with no "original" name. For these purely synthetic names, preserving whatever was first generated for them is probably not useful?
| auto name = new IR::ID(orig->getName().srcInfo, newName, orig->getName().originalName); | ||
| auto newName = renameMap->get(orig); | ||
| if (!newName.has_value()) return nullptr; | ||
| auto name = new IR::ID(orig->getName().srcInfo, *newName, orig->getName().originalName); |
There was a problem hiding this comment.
originalName should always be set for IDs that correspond to things in the source code -- originalName == nullptr implies that this is a purely compiler generated symbol with no "original" name. For these purely synthetic names, preserving whatever was first generated for them is probably not useful?
This is a slight generalization and refactor of
P4::RenameMap. The existing interface stays the same, but there are two more ways to use it:setNewNamecan now accept an optionalboolargument. If set totrue, the bug check for the case an an existing renaming is going to be replaced is disabled.getmember function that returnsstd::optional<cstring>was added. If the name has no mapping, this will returnstd::nullopt, while ingetNamethere was a failure. This allows the user of the map to skip double searching the map.