[PATCH] D78929: [AIX][XCOFF]emit extern linkage for the llvm intrinsic symbol

Wed May 6 08:03:00 PDT 2020

DiggerLin added inline comments.

================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1756
+  bool Ret = AsmPrinter::doFinalization(M);
+  for (auto Sym : IntrinsicSymbols)
+    OutStreamer->emitSymbolAttribute(Sym, MCSA_Extern);
----------------
jhenderson wrote:
> hubert.reinterpretcast wrote:
> > DiggerLin wrote:
> > > jhenderson wrote:
> > > > Don't use `auto` here. Also, should this really be a copy and not a const reference?
> > > it store the a pointer of MCSymbol in the IntrinsicSymbols. I think return a copy of pointer is same efficient as const reference. 
> > > 
> > > and the 
> > > 
> > > ```
> > > bool MCXCOFFStreamer::emitSymbolAttribute(MCSymbol *Sym,
> > >                                           MCSymbolAttr Attribute)
> > > ```
> > One issue with `auto` here is that it's not obvious that we have a pointer type (and even knowing how `Sym` is used does not rule out the possibility that it is merely convertible to the pointer type). Using `auto *` might not have caused the comment to be raised, but since the comment has been raised, please get rid of the `auto`.
> By the way, there's a related style guide section on usage of `auto`: https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
thanks for the information.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78929