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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
