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

[Michał Górny via Phabricator via cfe-commits]

mgorny marked 4 inline comments as done.
mgorny 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.
----------------
sepavloff wrote:
> I would say this paragraph is a combination of two provided below. IMHO.
Yeah, it was intentional. I was thinking it would be cleaner to describe this in tandem but I won't insist. Just please confirm whether I should remove it or keep it.


================
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
----------------
sepavloff wrote:
> 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.
Updated.


================
Comment at: clang/lib/Driver/Driver.cpp:1089
+
+  bool ModeSuffixUnique = !ClangNameParts.ModeSuffix.empty() &&
+                          ClangNameParts.ModeSuffix != RealMode.str();
----------------
sepavloff wrote:
> Can the variable name be better? It looks like it means that driver mode is replaced (overridden).
Can you suggest a better name? It's used to avoid searching for the same filename twice.


================
Comment at: clang/lib/Driver/Driver.cpp:6195
+
+  assert(false && "Unhandled Mode");
+  return "clang";
----------------
MaskRay wrote:
> MaskRay wrote:
> > If the switch covers all enum members, we typically use llvm_unreachable to work around a GCC -Wswitch warning.
> > Clang correctly knows all branches are covered and do not need `llvm_unreachable`
> `llvm_unreachable`
Right, sorry, forgot about it.


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

https://reviews.llvm.org/D134337