[PATCH] D75866: [AIX] supporting the visibility attribute for aix assembly

[Digger via Phabricator via llvm-commits]

DiggerLin marked 14 inline comments as done.
DiggerLin added inline comments.


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:33
 #include "llvm/MC/MCStreamer.h"
+#include "llvm/MC/MCSymbolXCOFF.h"
 #include "llvm/Support/ErrorHandling.h"
----------------
jasonliu wrote:
> how many of these newly added include directive are actually needed now?
thanks


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:802
+void MCAsmStreamer::emitXCOFFSymbolLinkageWithVisibility(
+    MCSymbol *Symbol, MCSymbolAttr Linkage, MCSymbolAttr Visibility) {
+
----------------
jasonliu wrote:
> Could we add `const` for the `Symbol` argument?
sorry , I can not add const in this patch, for  in the function 
MCXCOFFStreamer::emitXCOFFSymbolLinkageWithVisibility()

the function calls the 
bool MCXCOFFStreamer::emitSymbolAttribute(MCSymbol *Sym,Symbol, Visibility)

. if we want to change to " const  MCSymbol *Sym" , we can add a NFC patch later.


================
Comment at: llvm/lib/MC/MCXCOFFStreamer.cpp:57
+
+  ///< Not emit default visibility.
+  if (Visibility == MCSA_Invalid)
----------------
jasonliu wrote:
> jasonliu wrote:
> > hubert.reinterpretcast wrote:
> > > The comment here should not be Doxygen style. There should be a comment elsewhere (in the header for the base class declaration) that //is// in Doxygen style. Also, "default visibility" is ambiguous for XCOFF.
> > > 
> > > Suggestion:
> > > ```
> > >   // When the caller passes `MCSA_Invalid` for the visibility, do not emit one.
> > > ```
> > > There should be a comment elsewhere (in the header for the base class declaration) that is in Doxygen style.
> > I'm not seeing this comment being addressed?
> I don't think removing 
> ```
> if (Visibility == MCSA_Invalid)
>     return;
> ```
> before emit symbol attribute for visibility, and skip the requested comment is a good way to move forward.
> Without those, there is no clear contract to tell people how to use this function. 
if Visibility is  MCSA_Invalid, it will not come to function emitXCOFFSymbolLinkageWithVisibility


Repository:
  rZORG LLVM Github Zorg

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

https://reviews.llvm.org/D75866