[PATCH] D34947: [clangd] Add support for per-file extra flags

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/GlobalCompilationDatabase.cpp:20
+                          const SmallVectorImpl<StringRef> &ExtraFlags) {
+  assert(Command && !Command->CommandLine.empty());
+  if (!Command || ExtraFlags.empty())
----------------
Maybe pass `Command` by reference to avoid checking for null?
Is there a guideline to use pointers to make things more explicit that I've missed?


================
Comment at: clangd/GlobalCompilationDatabase.cpp:21
+  assert(Command && !Command->CommandLine.empty());
+  if (!Command || ExtraFlags.empty())
+    return;
----------------
There's an assert that `Command` is always non-null, so `!Command` is always `false`.
Maybe remove this check?


================
Comment at: clangd/GlobalCompilationDatabase.cpp:23
+    return;
+  if (Command->CommandLine.empty())
+    Command->CommandLine.push_back("clang");
----------------
There's an assert that checks that `CommandLine` is never empty.
Maybe remove this if statement?


================
Comment at: clangd/GlobalCompilationDatabase.cpp:27
+  ++It;
+  Command->CommandLine.insert(It, ExtraFlags.begin(), ExtraFlags.end());
+}
----------------
This way extra flags won't override flags from compilation database, we should put `ExtraFlags` right before the input files.

clangd will break if it receives CommandLine with more than one input file, so it should be safe to assume that the last arg is the name of the input file, i.e. always add `ExtraFlags` right before the last arg.


================
Comment at: clangd/GlobalCompilationDatabase.cpp:38
+  if (Commands.empty()) {
+    std::vector<std::string> CommandLine{"clang", "-fsyntax-only", File.str()};
+    Commands.push_back(tooling::CompileCommand(
----------------
Maybe extract a small helper function to get default `CompileCommand` and use it both in `ClangdUnitStore` and here?
Just in case we'll ever want to change the defaults at some point.



================
Comment at: clangd/GlobalCompilationDatabase.cpp:47
+    SmallVector<StringRef, 4> UserFlags;
+    StringRef(It->second).split(UserFlags, " ");
+    // Append the user-specified flags to the compile commands.
----------------
This will break for options, containing spaces (e.g. `-I"/home/user/my dir with spaces"`). 
There should a function parsing command-line args somewhere in LLVM? Can we avoid splitting command-line args altogether (see next comment)?


================
Comment at: clangd/GlobalCompilationDatabase.cpp:58
+    PathRef File, llvm::StringRef ExtraFlags) {
+  ExtraFlagsForFile[File] = ExtraFlags;
 }
----------------
Maybe store splitted command-line args in `ExtraFlagsForFile`?
`ycm_extra_conf.py` seems to pass splited command-line arguments, maybe change our extension to receive a list of arguments to avoid doing command-line arg splitting?


https://reviews.llvm.org/D34947