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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
