[PATCH] D87451: add new clang option -mno-xcoff-visibility

[David Tenty via Phabricator via llvm-commits]

daltenty added inline comments.


================
Comment at: llvm/include/llvm/Target/TargetMachine.h:265
+  /// corresponding to -mno-xcoff-visibility.
+  bool getNoXCOFFVisibility() const { return Options.NoXCOFFVisibility; }
+
----------------
DiggerLin wrote:
> jasonliu wrote:
> > DiggerLin wrote:
> > > daltenty wrote:
> > > > This seems like it needs the corresponding comand-line option for llc added and an llc test.
> > > I think it will be in another separate  patch.
> > I would actually prefer to have that in the same patch, as that would give us a full picture. It's not a huge patch even if we combine them. 
> yes, it is not huge patch, one patch for the clang with option -mno-xcoff-visibility, another patch for llc option -no-xcoff-visibility , I think it is different functionality. and I have post the https://reviews.llvm.org/D88234 "add new option -no-xcoff-visibility for llc"
But the problem is this patch actually introduces the backend functionality with no test in the LLVM component itself, because the test here is Clang only. Either the patches should be merged so both components get tests in one patch or both refactored so we have one llc/LLVM patch and one clang patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87451