[PATCH] D46394: [clang-cl] Print /showIncludes to stderr, if used in combination with /E, /EP or /P

Thu May 3 08:49:08 PDT 2018

fxb added inline comments.

================
Comment at: include/clang/Frontend/DependencyOutputOptions.h:31
   unsigned AddMissingHeaderDeps : 1; ///< Add missing headers to dependency list
   unsigned PrintShowIncludes : 1; ///< Print cl.exe style /showIncludes info.
+  unsigned
----------------
erichkeane wrote:
> Doing these two as separate options is a touch annoying.  First, there is an extra state that ends up being possible but ignored.  I'd prefer making a "PrintShowIncludeDestination : 2" (name open for proper bikeshed) that contains an enum so that the states are explicit.  Something like:
> None,
> StdOut,
> StdErr
> 
> That way, the 4th state is explicitly unused.
Yes, that definitely makes sense and is a lot nicer than having these two options, which I found quite ugly as well.

I will fix that and upload a new patch set.

================
Comment at: test/Driver/cl-options.c:203
-
-// RUN: %clang_cl /EP /P /showIncludes -### -- %s 2>&1 | FileCheck -check-prefix=showIncludes_E_And_P %s
-// showIncludes_E_And_P-NOT: warning: argument unused during compilation: '--show-includes'
----------------
erichkeane wrote:
> I'm perhaps missing something here... why did "/EP /P /showIncludes" previously NOT warn?  
/P will preprocess to a file, so it was compatible with /showIncludes, which was printed to stdout.

With this change all preprocessor options are now possible to use in combination with /showIncludes, since its output will be written to stderr, like cl.exe does.

I was actually not sure if I shall keep these tests at all after my change or just invert them, like I ended up doing here. There is probably a better way to actually test that /showIncludes output will end up on stderr, but with my limited experience I didn't figure that out yet. Let me know if you have any ideas how to improve these tests.

Repository:
  rC Clang

https://reviews.llvm.org/D46394