[PATCH] D71228: [Commandline] Move general LLVMSupport options into Commandline.cpp [NFC]

[serge via Phabricator via llvm-commits]

serge-sans-paille added a comment.

Giving some extra thoughts to that PR. I'm not a big fan of giving external visibility to flags like `DisableSymbolicationFlag` or `DebugBufferSize`. I certainly see value in activating them only on debug mode, and that could be a standalone patch.

Concerning the added value of having no options in `LLVMSupport`, isn't it an abstraction breakage to have debug-related stuff in the commandline implementation? It doesn't make more sense to me to have them in CL than in Support... And as you stated in the description, this doesn't solve the problem with other `cl::...` options in LLVMSupport.

If we want an option-less version of LLVMSupport, then I'd advocate for static variables with  static initializer + getter/setter in `LLVMSupport`, and a separate library , say `LLVMSupportCLOptions` that would just use the setter/getter to properly modify `LLVMSupport` behavior. Just my opinion though, you probably want more insights from more people.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71228