[PATCH] D24933: Enable configuration files in clang

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added inline comments.


================
Comment at: include/clang/Driver/Driver.h:287
+  const std::string &getConfigFile() const { return ConfigFile; }
+  void setConfigFile(StringRef x, unsigned N) {
+    ConfigFile = x;
----------------
bruno wrote:
> x -> FileName
Fixed.


================
Comment at: lib/Driver/Driver.cpp:172
+    NumConfigArgs = static_cast<unsigned>(NumCfgArgs);
+  }
+
----------------
bruno wrote:
> If `NumCfgArgs == 0` it would be nice to warn that the config file is empty?
Not sure if it makes sense. Usually warning are used to attract attention to the things that potentially may be harmful. An empty config file is strange but does not look dangerous. 


================
Comment at: lib/Driver/Driver.cpp:2698
+  for (unsigned I = 0; I < NumConfigArgs; ++I)
+    C.getArgs().getArgs()[I]->claim();
+
----------------
bruno wrote:
> Why shouldn't we warn about those? Should clang warn and point to the config file instead?
If clang is called to compile files only, it will warn on options like `-L/usr/local/lib` as they are unused. We don't want such warnings for options in config file, as it is used for all invocations of clang.


================
Comment at: tools/driver/driver.cpp:318
+
+/// Deduce configuration name if it is encoded in the executable name.
+///
----------------
bruno wrote:
> Use  `\brief` here?
Doxygen is run with AUTO_BRIEF options turned on (enabled in r242485 - Doxygen: Enable autobrief feature, matching llvm config/coding standards).


================
Comment at: tools/driver/driver.cpp:333
+  size_t Pos = PName.find("-clang");
+  if (Pos != StringRef::npos) {
+    ConfigFile.append(PName.begin(), PName.begin() + Pos);
----------------
bruno wrote:
> You can early `return false` here if `Pos == StringRef::npos`
Fixed.


https://reviews.llvm.org/D24933