[PATCH] D130426: [CodeGen] Fixed ambiguous symbol ExtAddrMode in case of NDEBUG and LLVM_ENABLE_DUMP

[Sergei Barannikov via Phabricator via llvm-commits]

barannikov88 added inline comments.


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:2618
 
+} // end anonymous namespace
+
----------------
aaron.ballman wrote:
> barannikov88 wrote:
> > slydiman wrote:
> > > barannikov88 wrote:
> > > > slydiman wrote:
> > > > > aaron.ballman wrote:
> > > > > > +1 to not extending the anonymous namespace like this but instead using a qualified identifier as necessary.
> > > > > You do realize that one cannot provide qualified name for unnamed namespace, right? But please feel free to explain if I’m missing something in your comment.
> > > > > I don’t see how this change possibly violates the developers policy, since this is exactly what is done here - a class declared in an unnamed namespace along with all its memebrs to fix the defect of possible referencing symbols from outside of this CU.
> > > > > As another approach, we could make that a named namespace, and use qualified names instead. This would also change the symbol visibility/linkage, which I personally do not like.
> > > > @slydiman 
> > > > Have you been able to find the place where TargetInstrInfo.h is included? Or is it some MSVC quirk that looks arcoss CUs?
> > > > 
> > > Unfortunately, I did not find the exact place. 
> > > I can reproduce the problem on one of my setups.
> > > It looks like a MSVC quirk around precompiled headers.
> > > As another approach, we could just rename `ExtAddrMode` to something like `ExtTargetLoweringAddrMode` keeping the anonymous namespace untouched.
> > Renaming TII's sounds reasonable to me. Something as general as "ExtAddrMode" should not have been added to llvm namespace in the first place (it is not that fundamental class to do so).
> > 
> > Another option is to try to move the class into TargetInstrInfo class declaration.
> > 
> > Either of these option will require changing backends.
> > 
> > If the intention is to just to make it work, renaming the local class seems to be much easier task. Or, alternatively, you could introduce another namespace under the anonymous one. This will keep the class hidden, while disambiguating it with TII's class. You don't see this very often though.
> > 
> > You do realize that one cannot provide qualified name for unnamed namespace, right? But please feel free to explain if I’m missing something in your comment.
> 
> I was thinking we'd be dropping the `using namespace llvm;` but I realize now, that's going to be fragile and not really fix the underlying issue. Sorry for the noise there!
> 
> I'm actually fine with these changes, the alternatives require more invasive changes that I don't think are worth it.

> I'm actually fine with these changes, the alternatives require more invasive changes that I don't think are worth it.

>From that point of view, looks good to me, too.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130426/new/

https://reviews.llvm.org/D130426