[PATCH] D134337: [clang] [Driver] More flexible rules for loading default configs

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added inline comments.


================
Comment at: clang/docs/ReleaseNotes.rst:236-239
+- Clang now supports loading multiple configuration files. The files from
+  default configuration paths are loaded first, unless ``--no-default-config``
+  option is used. All files explicitly specified using ``--config`` option
+  are loaded afterwards.
----------------
I would say this paragraph is a combination of two provided below. IMHO.


================
Comment at: clang/docs/ReleaseNotes.rst:240
+  are loaded afterwards.
+- Default configuration paths were partially changed. Clang now attempts to load
+  ``<target>-<driver>.cfg`` first, and falls back to loading both
----------------
This paragraph is actually about two changes. One is about loading driver mode file, this is an extension. The other is about skipping files using original target prefix. It is potentially breaking change and is worth separate paragraph.


================
Comment at: clang/docs/UsersManual.rst:918
+First, the algorithm searches for a configuration file named
+``<target>-<driver>.cfg`` where `target` is the triple for the target being
+built for, and `driver` is the name of the currently used driver. The algorithm
----------------
I would propose changing `<target>`->`<triple>` and `target`->`triple` to reduce misunderstanding.


================
Comment at: clang/docs/UsersManual.rst:949-950
+If none of the aforementioned files are found, the driver will instead search
+for separate target and driver configuration files and attempt to load both.
+The former is named ``<target>.cfg`` while the latter is named
+``<driver>.cfg``. Similarly to the previous variants, the canonical driver name
----------------
Maybe `target`->`triple`, `<target.cfg>`->`<triplr.cfg>`?


================
Comment at: clang/docs/UsersManual.rst:964
+    clang-g++.cfg
 
 The configuration file consists of command-line options specified on one or
----------------
I think we need to put something like:
It is not an error if any or both of these files are not found.


================
Comment at: clang/lib/Driver/Driver.cpp:1089
+
+  bool ModeSuffixUnique = !ClangNameParts.ModeSuffix.empty() &&
+                          ClangNameParts.ModeSuffix != RealMode.str();
----------------
Can the variable name be better? It looks like it means that driver mode is replaced (overridden).


================
Comment at: clang/test/Driver/config-file3.c:33
+// RUN: ln -s %clang %t/testdmode/x86_64-unknown-linux-gnu-clang
+// RUN: echo > %t/testdmode/x86_64-unknown-linux-gnu-clang++.cfg
+// RUN: echo > %t/testdmode/x86_64-unknown-linux-gnu-clang-g++.cfg
----------------
mgorny wrote:
> pengfei wrote:
> > MaskRay wrote:
> > > Since %t is rebuilt. You can just use `touch`
> > `touch` may not work on Windows?
> Well, my experience tells me to avoid external commands when the goal can be achieved via a builtin but `touch` is more popular in clang indeed.
IIRC, `shell` is not available on Windows.


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

https://reviews.llvm.org/D134337