<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Sep 20, 2016 at 4:21 PM, Albert Gutowski <span dir="ltr"><<a href="mailto:agutowski@google.com" target="_blank">agutowski@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">OK, thanks for the note about referring to Chromium, I fixed that.<br>As to -Wunknown-pragma, I feel that it would be inconsistent with other pragmas unless I moved whole pragma to lexer instead of parser - I've just discovered that I can do that, how should I decide if it's supposed to be here or there?<br></div></blockquote><div><br></div><div>Your commit reminds me that I forgot to reply to this thread. For the lexer/parser split, I think about it like this: If clang doesn't know about a certain pragma at all, it can't do anything with it, so all it does is emit 'unknown pragma', skip it, and move on. If it does know the pragma, it can parse it -- but then it might have to ignore it for some reason, which is why the parser prints that diagnostic.</div><div><br></div><div>So you're right that this doesn't fit in well in -Wunknown-pragma.</div><div><br></div><div>But -Wignored-pragma doesn't quite fit either imho, since we're not ignoring the pragma, we're looking at its context.</div><div><br></div><div>Applying the same idea for pragmas to `#pragma intrinsic`, it feels to me that we don't know some intrinsics at all, so they're more unknown than ignored (as in -Wunkown-pragma-intrinsic, but in the sense of as "unknown (pragma intrinsic)" not "(unknown pragma) intrinsic" -- but since this shouldn't be in -Wunknown-pragma as you correctly say, that's not a good name for this flag.)</div><div><br></div><div>So I think -Wmicrosoft-pragma-intrinsic might be a better fit. (But as I said, it's bikeshedding, and up to you.)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Sep 20, 2016 at 12:30 PM, Nico Weber <span dir="ltr"><<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Thanks! Some bikesheddy comments, ignore as you see fit:<div><br></div><div>- I think it's good to keep Chromium's bug tracker out of LLVM as far as possible (most LLVM devs don't need to care about Chromium, and since Chromium's bug tracker refers to LLVM's bug tracker frequently since chromium depends on llvm, adding links the other way round adds a dependency cycle of sorts), so maybe describe the motivation in your own words instead of linking ("People might want to receive warnings about pragmas but not about intrinsics we don't yet implement")</div><div><br></div><div>- Should this be part of -Wunknown-pragma instead of -Wignored-pragma? (Or maybe even in -Wmicrosoft somewhere?) That seems to fit a tiny bit better imho (but as I said, feel free to disagree and ignore)</div></div><div class="m_-8909771502912946664HOEnZb"><div class="m_-8909771502912946664h5"><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Sep 20, 2016 at 2:38 PM, Albert Gutowski <span dir="ltr"><<a href="mailto:agutowski@google.com" target="_blank">agutowski@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">agutowski created this revision.<br>
agutowski added reviewers: thakis, hans.<br>
agutowski added a subscriber: cfe-commits.<br>
<br>
<a href="https://bugs.chromium.org/p/chromium/issues/detail?id=644841#c9" rel="noreferrer" target="_blank">https://bugs.chromium.org/p/ch<wbr>romium/issues/detail?id=644841<wbr>#c9</a><br>
<br>
<a href="https://reviews.llvm.org/D24775" rel="noreferrer" target="_blank">https://reviews.llvm.org/D2477<wbr>5</a><br>
<br>
Files:<br>
include/clang/Basic/Diagnostic<wbr>Groups.td<br>
include/clang/Basic/Diagnostic<wbr>ParseKinds.td<br>
test/Preprocessor/pragma_micro<wbr>soft.c<br>
<br>
Index: include/clang/Basic/Diagnostic<wbr>ParseKinds.td<br>
==============================<wbr>==============================<wbr>=======<br>
--- include/clang/Basic/Diagnostic<wbr>ParseKinds.td<br>
+++ include/clang/Basic/Diagnostic<wbr>ParseKinds.td<br>
@@ -914,7 +914,7 @@<br>
// - #pragma intrinsic<br>
def warn_pragma_intrinsic_builtin : Warning<<br>
"%0 is not a recognized builtin%select{|; consider including <intrin.h> to access non-builtin intrinsics}1">,<br>
- InGroup<IgnoredPragmas>;<br>
+ InGroup<IgnoredPragmaIntrinsic<wbr>>;<br>
// - #pragma unused<br>
def warn_pragma_unused_expected_va<wbr>r : Warning<<br>
"expected '#pragma unused' argument to be a variable name">,<br>
Index: include/clang/Basic/Diagnostic<wbr>Groups.td<br>
==============================<wbr>==============================<wbr>=======<br>
--- include/clang/Basic/Diagnostic<wbr>Groups.td<br>
+++ include/clang/Basic/Diagnostic<wbr>Groups.td<br>
@@ -439,8 +439,9 @@<br>
def UninitializedStaticSelfInit : DiagGroup<"static-self-init">;<br>
def Uninitialized : DiagGroup<"uninitialized", [UninitializedSometimes,<br>
UninitializedStaticSelfInit]>;<br>
+def IgnoredPragmaIntrinsic : DiagGroup<"ignored-pragma-intr<wbr>insic">;<br>
def UnknownPragmas : DiagGroup<"unknown-pragmas">;<br>
-def IgnoredPragmas : DiagGroup<"ignored-pragmas">;<br>
+def IgnoredPragmas : DiagGroup<"ignored-pragmas", [IgnoredPragmaIntrinsic]>;<br>
def Pragmas : DiagGroup<"pragmas", [UnknownPragmas, IgnoredPragmas]>;<br>
def UnknownWarningOption : DiagGroup<"unknown-warning-opt<wbr>ion">;<br>
def NSobjectAttribute : DiagGroup<"NSObject-attribute"<wbr>>;<br>
Index: test/Preprocessor/pragma_micro<wbr>soft.c<br>
==============================<wbr>==============================<wbr>=======<br>
--- test/Preprocessor/pragma_micro<wbr>soft.c<br>
+++ test/Preprocessor/pragma_micro<wbr>soft.c<br>
@@ -178,3 +178,15 @@<br>
#pragma intrinsic(memset) // no-warning<br>
#undef __INTRIN_H<br>
#pragma intrinsic(asdf) // expected-warning {{'asdf' is not a recognized builtin; consider including <intrin.h>}}<br>
+<br>
+#pragma clang diagnostic push<br>
+#pragma clang diagnostic ignored "-Wignored-pragma-intrinsic"<br>
+#pragma intrinsic(asdf) // no-warning<br>
+#pragma clang diagnostic pop<br>
+#pragma intrinsic(asdf) // expected-warning {{'asdf' is not a recognized builtin; consider including <intrin.h>}}<br>
+<br>
+#pragma clang diagnostic push<br>
+#pragma clang diagnostic ignored "-Wignored-pragmas"<br>
+#pragma intrinsic(asdf) // no-warning<br>
+#pragma clang diagnostic pop<br>
+#pragma intrinsic(asdf) // expected-warning {{'asdf' is not a recognized builtin; consider including <intrin.h>}}<br>
<br>
<br>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>
</div></div></blockquote></div><br></div></div>