[PATCH] D42887: [Driver] Add option to manually control discarding value names in LLVM IR.

[Eric Fiselier via Phabricator via cfe-commits]

EricWF marked 5 inline comments as done.
EricWF added inline comments.


================
Comment at: lib/Driver/ToolChains/Clang.cpp:3269-3274
+  const bool IsAssertBuild =
 #ifdef NDEBUG
-  CmdArgs.push_back("-disable-llvm-verifier");
-  // Discard LLVM value names in -asserts builds.
-  CmdArgs.push_back("-discard-value-names");
+      false;
+#else
+      true;
 #endif
----------------
bogner wrote:
> It might be a few more characters, but I feel like this is more readable if you put entire statements in the branches of the #if, ie:
> 
> #ifdef NDEBUG
>   const bool IsAssertBuild = false;
> #else
>   const bool IsAssertBuild = true;
> #endif
Ack. Done.


================
Comment at: test/Driver/clang_f_opts.c:522
+// RUN: %clang -### -S -fdiscard-value-names %s 2>&1 | FileCheck -check-prefix=CHECK-DISCARD-NAMES %s
+// RUN: %clang -### -S -fno-discard-value-names %s 2>&1 | FileCheck -check-prefix=CHECK-NO-DISCARD-NAMES %s
+// CHECK-DISCARD-NAMES: "-discard-value-names"
----------------
lebedev.ri wrote:
> I wonder if it is also possible to check that if neither of `-f[no-]discard-value-names` is specified, what happens.
> The caveat is of course the asserts-vs-no-asserts build type.
I don't think so, at least not easily and without changes to the `lit` configuration.

It's gone untested this long, I would love to get this patch in without being responsible for adding those tests.



https://reviews.llvm.org/D42887