[PATCH] D71080: [NFC] Separate getLastArgIntValue to Basic

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 5 inline comments as done.
yaxunl added inline comments.


================
Comment at: clang/include/clang/Basic/OptionUtils.h:24
+
+class ArgList;
+
----------------
tra wrote:
> What are the rules on using LLVM headers here? Can we just include llvm/Option/ArgList.h instead?
In the API llvm::opt::ArgList is used as reference, therefore it only needs a forward declaration. llvm::opt::OptSpecifier is passed by value, therefore it needs header. OptSpecifier is a small class, therefore it is not efficient to pass by reference.


================
Comment at: clang/include/clang/Basic/OptionUtils.h:46
+                               DiagnosticsEngine *Diags = nullptr,
+                               unsigned Base = 10);
+
----------------
tra wrote:
> Same question as before -- does it have to be `10`?
> `0` would be a more reasonable default for general use. IMO we care about the value, but not so much about the form. I.e. is there a reason not to allow 0xf, for instance, if that works for the user?
> 
will do. StringRef will do radix autosensing for that.


================
Comment at: clang/lib/Basic/CMakeLists.txt:1
 set(LLVM_LINK_COMPONENTS
   Core
----------------
tra wrote:
> I think now that you're using ArgList, you need to depend on LLVM's LLVMOption library.
> As is you're likely to run into build issues if shared libraries are enabled.
will do


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

https://reviews.llvm.org/D71080