In "using WrEpoch = Epoch", Epoch does not link to its definition because of symbol collision with a typedef
Categories
(Webtools :: Searchfox, defect)
Tracking
(Not tracked)
People
(Reporter: mstange, Unassigned)
Details
(Keywords: leave-open)
Attachments
(1 file)
webrender_ffi_generated.h currently contains this code:
struct Epoch {
uint32_t mHandle;
bool operator==(const Epoch& aOther) const {
return mHandle == aOther.mHandle;
}
bool operator!=(const Epoch& aOther) const {
return mHandle != aOther.mHandle;
}
bool operator<(const Epoch& aOther) const {
return mHandle < aOther.mHandle;
}
bool operator<=(const Epoch& aOther) const {
return mHandle <= aOther.mHandle;
}
};
using WrEpoch = Epoch;
struct WrPipelineEpoch {
WrPipelineId pipeline_id;
WrDocumentId document_id;
WrEpoch epoch;
bool operator==(const WrPipelineEpoch& aOther) const {
return pipeline_id == aOther.pipeline_id &&
document_id == aOther.document_id &&
epoch == aOther.epoch;
}
};
I was trying to go from the line "WrEpoch epoch;" all the way to the struct definition, by successively clicking "go to definition". However, I got stuck at the "using WrEpoch = Epoch;" line: "Epoch" does not have a "Go to definition" menu item. I could just have scrolled up a few more lines but I didn't see it.
Comment 1•4 years ago
|
||
It seems like the problem is the typedef here causes there to be more than one definition for the symbol T_mozilla::wr::Epoch
which causes the heuristics that emit "go to definition" to give up because there can only be one!
typedef wr::WrEpoch Epoch;
The analysis data for the using lines (spread over two ranges because of merge-analyses):
{"loc":"00705:16-21","target":1,"kind":"use","pretty":"mozilla::wr::Epoch","sym":"T_mozilla::wr::Epoch"}
{"loc":"00705:6-13","target":1,"kind":"def","pretty":"mozilla::wr::WrEpoch","sym":"T_mozilla::wr::WrEpoch"}
{"loc":"00705:6-13","source":1,"syntax":"def,type","pretty":"type mozilla::wr::WrEpoch","sym":"T_mozilla::wr::WrEpoch"}
{"loc":"00705:16-21","source":1,"syntax":"type,use","pretty":"type mozilla::wr::Epoch","sym":"T_mozilla::wr::Epoch"}
The analysis data for the Epoch line is:
{"loc":"00688:7-12","target":1,"kind":"def","pretty":"mozilla::wr::Epoch","sym":"T_mozilla::wr::Epoch","peekRange":"684-688"}
{"loc":"00688:7-12","source":1,"syntax":"def,type","pretty":"type mozilla::wr::Epoch","sym":"T_mozilla::wr::Epoch","nestingRange":"688:13-703:0"}
The analysis data for the typedef line in WebRenderTypes.h is:
{"loc":"00048:12-19","target":1,"kind":"use","pretty":"mozilla::wr::WrEpoch","sym":"T_mozilla::wr::WrEpoch"}
{"loc":"00048:20-25","target":1,"kind":"def","pretty":"mozilla::wr::Epoch","sym":"T_mozilla::wr::Epoch"}
{"loc":"00048:12-19","source":1,"syntax":"type,use","pretty":"type mozilla::wr::WrEpoch","sym":"T_mozilla::wr::WrEpoch"}
{"loc":"00048:20-25","source":1,"syntax":"def,type","pretty":"type mozilla::wr::Epoch","sym":"T_mozilla::wr::Epoch"}
Updated•4 years ago
|
Comment 2•4 years ago
|
||
We should probably just remove that typedef since the file already includes webrender_ffi_generated.h via webrender_ffi.h.
Comment 3•4 years ago
|
||
These typedefs seem redundant given the using
statements in
the webrender_ffi_generated.h header.
Updated•4 years ago
|
Comment 4•4 years ago
|
||
That definitely sounds like an appropriate codebase fix. I guess the question for searchfox is whether the context menu logic should do something more helpful here than not including "go to definition" and/or the search results could be improved.
Searchfox is failing to make it clear why there's no definition in the context menu and when a user picks the search results, the partitioning of the definitions into "normal" (no special heading) and "generated" makes it hard to immediately realize that there's a symbol collision.
Options might include:
- Instead of omitting "go to definition" include "search for 2 definitions" and have that use a new search constraint to limit definitions (and not do the redirect trick that the 'define' path induces). This maintains the shape of the left-click menu, makes it clear that there's multiple definitions, and reduces the manual effort required to locate both definitions.
- It's actually crossref.rs that decides not to emit jumps if there's more than one definition, so this would want to instead emit an alternate jump record that conveys the number of definitions that exist.
- Then the jumps emission logic in format.rs wants to help propagate the extra info too.
- And the context-menu.js logic that renders it would want to detect the slightly different condition and change the string and search URI generated.
- router.py's result logic needs to learn to detect the extra search parameter and only include the contents of the 'Definitions', possibly via altering the behavior of SearchResults.add_results
- On definitions that have a colliding definition, we have logic that avoids emitting the definition jump in format.rs but this could instead emit a specialized jump record that could alter the above logic to instead say something like "Search colliding definitions" or "Search equivalent definitions" or something along those lines.
Pushed by kgupta@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/583ecfdab160 Remove unnecessary typedefs. r=mstange
Reporter | ||
Comment 6•4 years ago
|
||
Why does the compiler not reject the duplicate definition? Is it because both definitions end up being the same? So would it be possible to have a similar same-ness check in searchfox?
Comment 7•4 years ago
|
||
(In reply to Markus Stange [:mstange] from comment #6)
Why does the compiler not reject the duplicate definition? Is it because both definitions end up being the same?
This sounds like something :botond would know the answer to!
So would it be possible to have a similar same-ness check in searchfox?
Ah, so my comment 4 proposals still probably make some sense, but I think the issue you raise he reveals my analysis in comment 1 glossed over the root problem.
Searchfox is mangling symbol names for structs/unions the same as typedefs. We probably want to give typedefs their own mangled name that's distinct as the current mangling seems destined to cause collisions that break "go to definition". Especially since historically the typedef thing is a very common idiom to avoid having to type "struct" like a sucker. The current logic is:
else if (isa<TagDecl>(Decl) || isa<TypedefNameDecl>(Decl) ||
isa<ObjCInterfaceDecl>(Decl)) {
if (!Decl->getIdentifier()) {
// Anonymous.
return std::string("T_") + mangleLocation(Decl->getLocation());
}
return std::string("T_") + mangleQualifiedName(getQualifiedName(Decl));
Updated•4 years ago
|
Comment 8•4 years ago
|
||
(In reply to Andrew Sutherland [:asuth] (he/him) from comment #7)
(In reply to Markus Stange [:mstange] from comment #6)
Why does the compiler not reject the duplicate definition? Is it because both definitions end up being the same?
This sounds like something :botond would know the answer to!
Here's what the standard says about it ([dcl.typedef] p3):
In a given non-class scope, a
typedef
specifier can be used to redeclare the name of any type declared in that scope to refer to the type to which it already refers. [Example 3:
typedef struct s { /* ... */ } s;
typedef int I;
typedef int I;
typedef I I;
— end example]
Comment 9•4 years ago
|
||
I agree with comment 4 and comment 7 - there are definitely better ways for searchfox to handle these cases. However I don't anticipate working on this beyond the patch I landed, so I'm going to unassign this bug for now.
Comment 10•4 years ago
|
||
bugherder |
Description
•