Open Bug 1665501 Opened 4 years ago Updated 4 years ago

In "using WrEpoch = Epoch", Epoch does not link to its definition because of symbol collision with a typedef

Categories

(Webtools :: Searchfox, defect)

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.

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"}
Summary: In "using WrEpoch = Epoch", Epoch does not link to its definition → In "using WrEpoch = Epoch", Epoch does not link to its definition because of symbol collision with a typedef

We should probably just remove that typedef since the file already includes webrender_ffi_generated.h via webrender_ffi.h.

These typedefs seem redundant given the using statements in
the webrender_ffi_generated.h header.

Assignee: nobody → kats
Status: NEW → ASSIGNED

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:

Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/583ecfdab160
Remove unnecessary typedefs. r=mstange

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?

(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));
Flags: needinfo?(botond)

(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]

Flags: needinfo?(botond)

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.

Assignee: kats → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: