[PATCH] D24926: Added support of configuration files

[Serge Pavlov via llvm-commits]

sepavloff marked 2 inline comments as done.

================
Comment at: lib/Support/CommandLine.cpp:956
@@ +955,3 @@
+    FilePath.clear();
+    if (Dir[0] == '~') {
+      assert(llvm::sys::path::is_separator(Dir[1]));
----------------
ABataev wrote:
> What if Dir is nullptr?
It would be programmer's error. Added `assert`.

================
Comment at: lib/Support/CommandLine.cpp:977
@@ +976,3 @@
+                                 StringRef ToolName) {
+  StringRef ProgramFullPath = Argv[0];
+  CfgFileName.clear();
----------------
ABataev wrote:
> What if Argv is empty?
It would be programmers's error - `Argv` must represents `argv` from `main`. Added `assert`.

================
Comment at: lib/Support/CommandLine.cpp:984
@@ +983,3 @@
+  // specified by it.
+  auto cfg_option = std::find_if(Argv.begin(), Argv.end(), [](const char *x) {
+    return strcmp(x, "--config") == 0;
----------------
ABataev wrote:
> 'cfg_option' does not follow LLVM coding standard. And 'x'
Fixed.

================
Comment at: lib/Support/CommandLine.cpp:1000-1001
@@ +999,4 @@
+    if (!llvm::sys::path::has_parent_path(CfgFile)) {
+      if (!StringRef(CfgFile).endswith(".cfg"))
+        CfgFile += ".cfg";
+
----------------
ABataev wrote:
> Use llvm::sys::path::replace_extension(CfgFile, ".cfg")
That would change semantics. A user can use something like `testing.64` as a config name, it would be silently converted to `testing.cfg` which is not the desired behavior.

================
Comment at: lib/Support/CommandLine.cpp:1036
@@ +1035,3 @@
+      return CfgFileSearch::Successful;
+    else
+      return CfgFileSearch::Ignored;
----------------
ABataev wrote:
> Remove 'else', unconditional 'return CfgFileSearch::Ignored;'
Fixed.

================
Comment at: lib/Support/CommandLine.cpp:1142
@@ +1141,3 @@
+      if (*Cur == '\n' &&
+          !(Cur[-1] == '\\' || (Cur[-1] == '\r' && Cur[-2] == '\\')))
+        break;
----------------
ABataev wrote:
> What if Cur == Source.begin()?
This is an error. Rewrote this place, added unit tests for this function.


https://reviews.llvm.org/D24926