<div dir="ltr">Hi,<div><br></div><div>Thanks for the suggestions. I actually had tested with a fake header file (so I could build more interesting testcases) but it didn't occur to me to actually make it part of the codebase test.</div><div><br></div><div>I was afraid of being too kind to exempt PredefinedExpr symbols in any context, and that's why I only do it if the symbol appears on a system header. Had I exempted the symbol in any context, this would be allowed:<br><br>void some_user_function(const char*);<br>some_user_func(__PRETTY_FUNCTION__);</div><div><br></div><div>That's a judgment call, really. I went for the safe side, but wouldn't mind to change it at all. As you pointed we already allow the decay of string literals, so it may be the better way.</div><div><br></div><div>On the other hand, I went a little beyond the original scope. I added the precondition of not being a "system symbol decay in system header".</div><div>By "system symbol" I mean a PredefinedExpr or any symbol declared in a system header.</div><div><br></div><div>Let's do it like this: I'll create a header, include it passing the -isystem flag, and add several more tests and scenarios. I'll add the result the code after my changes produce, and we can judge on the concrete case what makes more sense.</div><div><br></div><div>Is that reasonable?</div><div><br></div><div>Best Regards,</div><div>Breno G.</div><div><br></div><div><br></div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Mar 21, 2017 at 11:37 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</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"><div class="gmail_extra"><div class="gmail_quote"><span class="">On 21 March 2017 at 18:57, Breno Guimarães via cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Hey guys,<div><br></div><div>I'm sorry the test did not make the decay explicit. It's indeed sort of tricky: <a href="https://bugs.llvm.org/show_bug.cgi?id=32239" target="_blank">https://bugs.llvm.org/<wbr>show_bug.cgi?id=32239</a></div><div><br></div><div>Basically, assert is expanded into:</div><div><pre class="m_6608578721823455276gmail-m_-5044091989478664758gmail-bz_comment_text" style="white-space:pre-wrap;width:50em;color:rgb(0,0,0)">(__builtin_expect(!(e), 0) ? __assert_rtn(__func__, __FILE__, __LINE__, #e) </pre></div></div></blockquote></span><div>Maybe in your libc, and even in most libc's, but not in general, and we can't and shouldn't rely on that in this test. (And there may not even *be* a usable <assert.h> in the include path...)</div><span class=""><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><pre class="m_6608578721823455276gmail-m_-5044091989478664758gmail-bz_comment_text" style="white-space:pre-wrap;width:50em;color:rgb(0,0,0)">Well, __func__ and __FILE__ are const char[], while __assert_rtn expects const char*.</pre></div></div></blockquote></span><div>I would think __func__ (and its friends __FUNCTION__, __PRETTY_FUNCTION__, etc) should be exempted from this warning just like decay on a string literal is. __FILE__ is string literal, so should be exempted already. Perhaps checking for a system header is not the right change at all?</div><span class=""><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><pre class="m_6608578721823455276gmail-m_-5044091989478664758gmail-bz_comment_text" style="white-space:pre-wrap;width:50em;color:rgb(0,0,0)">I've also seen this in my own environment (Linux) with slightly different function names, but basically the same problem.</pre><pre class="m_6608578721823455276gmail-m_-5044091989478664758gmail-bz_comment_text" style="white-space:pre-wrap;width:50em;color:rgb(0,0,0)">Any ideas of how to rewrite the test (or the code?) to have the change pushed?</pre></div></div></blockquote></span><div>For the current approach: add a header file (to Inputs/...) that contains the macro you want to use and add a -isystem path to the test pointing at your Inputs/... directory. To whitelist conversions of __func__ etc you should also check for PredefinedExpr wherever we're currently checking for StringLiteral.<br></div><div><div class="h5"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><pre class="m_6608578721823455276gmail-m_-5044091989478664758gmail-bz_comment_text" style="white-space:pre-wrap;width:50em;color:rgb(0,0,0)">Thanks!</pre><pre class="m_6608578721823455276gmail-m_-5044091989478664758gmail-bz_comment_text" style="white-space:pre-wrap;width:50em;color:rgb(0,0,0)">Breno G.</pre></div><div><br></div><div><br></div></div><div class="gmail_extra"><div><div class="m_6608578721823455276gmail-h5"><br><div class="gmail_quote">On Tue, Mar 21, 2017 at 10:36 PM, Richard Smith via cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Thank you!</div><div class="m_6608578721823455276gmail-m_-5044091989478664758HOEnZb"><div class="m_6608578721823455276gmail-m_-5044091989478664758h5"><div class="gmail_extra"><br><div class="gmail_quote">On 21 March 2017 at 18:22, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="m_6608578721823455276gmail-m_-5044091989478664758m_4153817042893638496HOEnZb"><div class="m_6608578721823455276gmail-m_-5044091989478664758m_4153817042893638496h5">On Tue, Mar 21, 2017 at 9:15 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> wrote:<br>
> On 21 March 2017 at 12:01, Aaron Ballman via cfe-commits<br>
> <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br>
>><br>
>> Author: aaronballman<br>
>> Date: Tue Mar 21 14:01:17 2017<br>
>> New Revision: 298421<br>
>><br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=298421&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject?rev=298421&view=rev</a><br>
>> Log:<br>
>> Prevent cppcoreguidelines-pro-bounds-a<wbr>rray-to-pointer-decay from<br>
>> diagnosing array to pointer decay stemming from system macros.<br>
>><br>
>> Patch by Breno Rodrigues Guimaraes.<br>
>><br>
>> Modified:<br>
>><br>
>> clang-tools-extra/trunk/clang-<wbr>tidy/cppcoreguidelines/ProBoun<wbr>dsArrayToPointerDecayCheck.cpp<br>
>><br>
>> clang-tools-extra/trunk/test/c<wbr>lang-tidy/cppcoreguidelines-pr<wbr>o-bounds-array-to-pointer-deca<wbr>y.cpp<br>
>><br>
>> Modified:<br>
>> clang-tools-extra/trunk/clang-<wbr>tidy/cppcoreguidelines/ProBoun<wbr>dsArrayToPointerDecayCheck.cpp<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/ProBoundsArrayToPointerDecayCheck.cpp?rev=298421&r1=298420&r2=298421&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/clang-tools-extra/trunk/<wbr>clang-tidy/cppcoreguidelines/P<wbr>roBoundsArrayToPointerDecayChe<wbr>ck.cpp?rev=298421&r1=298420&r2<wbr>=298421&view=diff</a><br>
>><br>
>> ==============================<wbr>==============================<wbr>==================<br>
>> ---<br>
>> clang-tools-extra/trunk/clang-<wbr>tidy/cppcoreguidelines/ProBoun<wbr>dsArrayToPointerDecayCheck.cpp<br>
>> (original)<br>
>> +++<br>
>> clang-tools-extra/trunk/clang-<wbr>tidy/cppcoreguidelines/ProBoun<wbr>dsArrayToPointerDecayCheck.cpp<br>
>> Tue Mar 21 14:01:17 2017<br>
>> @@ -47,6 +47,25 @@ AST_MATCHER_P(Expr, hasParentIgnoringImp<br>
>> return InnerMatcher.matches(*E, Finder, Builder);<br>
>> }<br>
>><br>
>> +AST_MATCHER(ImplicitCastExpr, isArrayToPointerDecay) {<br>
>> + return Node.getCastKind() == CK_ArrayToPointerDecay;<br>
>> +}<br>
>> +<br>
>> +AST_MATCHER(ImplicitCastExpr, sysSymbolDecayInSysHeader) {<br>
>> + auto &SM = Finder->getASTContext().getSou<wbr>rceManager();<br>
>> + if (SM.isInSystemMacro(Node.getLo<wbr>cStart())) {<br>
>> + if (isa<PredefinedExpr>(Node.getS<wbr>ubExpr()))<br>
>> + return true;<br>
>> +<br>
>> + if (const auto *SymbolDeclRef =<br>
>> dyn_cast<DeclRefExpr>(Node.get<wbr>SubExpr())) {<br>
>> + const ValueDecl *SymbolDecl = SymbolDeclRef->getDecl();<br>
>> + if (SymbolDecl && SM.isInSystemHeader(SymbolDecl<wbr>->getLocation()))<br>
>> + return true;<br>
>> + }<br>
>> + }<br>
>> + return false;<br>
>> +}<br>
>> +<br>
>> void ProBoundsArrayToPointerDecayCh<wbr>eck::registerMatchers(MatchFin<wbr>der<br>
>> *Finder) {<br>
>> if (!getLangOpts().CPlusPlus)<br>
>> return;<br>
>> @@ -56,10 +75,12 @@ void ProBoundsArrayToPointerDecayCh<wbr>eck::<br>
>> // 2) inside a range-for over an array<br>
>> // 3) if it converts a string literal to a pointer<br>
>> Finder->addMatcher(<br>
>> - implicitCastExpr(unless(hasPar<wbr>ent(arraySubscriptExpr())),<br>
>> + implicitCastExpr(isArrayToPoin<wbr>terDecay(),<br>
>> + unless(hasParent(arraySubscri<wbr>ptExpr())),<br>
>><br>
>> unless(hasParentIgnoringImpCas<wbr>ts(explicitCastExpr())),<br>
>> unless(isInsideOfRangeBeginEn<wbr>dStmt()),<br>
>> - unless(hasSourceExpression(st<wbr>ringLiteral())))<br>
>> + unless(hasSourceExpression(st<wbr>ringLiteral())),<br>
>> + unless(sysSymbolDecayInSysHea<wbr>der()))<br>
>> .bind("cast"),<br>
>> this);<br>
>> }<br>
>> @@ -67,8 +88,6 @@ void ProBoundsArrayToPointerDecayCh<wbr>eck::<br>
>> void ProBoundsArrayToPointerDecayCh<wbr>eck::check(<br>
>> const MatchFinder::MatchResult &Result) {<br>
>> const auto *MatchedCast =<br>
>> Result.Nodes.getNodeAs<Implici<wbr>tCastExpr>("cast");<br>
>> - if (MatchedCast->getCastKind() != CK_ArrayToPointerDecay)<br>
>> - return;<br>
>><br>
>> diag(MatchedCast->getExprLoc()<wbr>, "do not implicitly decay an array into<br>
>> a "<br>
>> "pointer; consider using<br>
>> gsl::array_view or "<br>
>><br>
>> Modified:<br>
>> clang-tools-extra/trunk/test/c<wbr>lang-tidy/cppcoreguidelines-pr<wbr>o-bounds-array-to-pointer-deca<wbr>y.cpp<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-bounds-array-to-pointer-decay.cpp?rev=298421&r1=298420&r2=298421&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/clang-tools-extra/trunk/<wbr>test/clang-tidy/cppcoreguideli<wbr>nes-pro-bounds-array-to-pointe<wbr>r-decay.cpp?rev=298421&r1=2984<wbr>20&r2=298421&view=diff</a><br>
>><br>
>> ==============================<wbr>==============================<wbr>==================<br>
>> ---<br>
>> clang-tools-extra/trunk/test/c<wbr>lang-tidy/cppcoreguidelines-pr<wbr>o-bounds-array-to-pointer-deca<wbr>y.cpp<br>
>> (original)<br>
>> +++<br>
>> clang-tools-extra/trunk/test/c<wbr>lang-tidy/cppcoreguidelines-pr<wbr>o-bounds-array-to-pointer-deca<wbr>y.cpp<br>
>> Tue Mar 21 14:01:17 2017<br>
>> @@ -1,4 +1,5 @@<br>
>> // RUN: %check_clang_tidy %s<br>
>> cppcoreguidelines-pro-bounds-a<wbr>rray-to-pointer-decay %t<br>
>> +#include <assert.h><br>
><br>
><br>
> This test is using a header that we do not supply (unlike stddef.h which<br>
> Clang provides itself). That does not seem especially reasonable to me; this<br>
> test is failing for us as a result. Can you supply a fake <assert.h> system<br>
> header as an input to this test?<br>
><br>
>><br>
>> #include <stddef.h><br>
>><br>
>> namespace gsl {<br>
>> @@ -34,6 +35,11 @@ void f() {<br>
>><br>
>> for (auto &e : a) // OK, iteration internally decays array to pointer<br>
>> e = 1;<br>
>> +<br>
>> + assert(false); // OK, array decay inside system header macro<br>
><br>
><br>
> Huh? What decay is this referring to?<br>
<br>
</div></div>I am now wondering the same question...<br>
<br>
I've rolled this back r298470 and will follow up with the author.<br>
<br>
Thanks!<br>
<span class="m_6608578721823455276gmail-m_-5044091989478664758m_4153817042893638496HOEnZb"><font color="#888888"><br>
~Aaron<br>
</font></span><div class="m_6608578721823455276gmail-m_-5044091989478664758m_4153817042893638496HOEnZb"><div class="m_6608578721823455276gmail-m_-5044091989478664758m_4153817042893638496h5"><br>
><br>
>> +<br>
>> + assert(a);<br>
>> + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not implicitly decay an<br>
>> array into a pointer; consider using gsl::array_view or an explicit cast<br>
>> instead [cppcoreguidelines-pro-bounds-<wbr>array-to-pointer-decay]<br>
>> }<br>
>><br>
>> const char *g() {<br>
>><br>
>><br>
>> ______________________________<wbr>_________________<br>
>> cfe-commits mailing list<br>
>> <a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
>> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a><br>
><br>
><br>
</div></div></blockquote></div><br></div>
</div></div><br>______________________________<wbr>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div><br><br clear="all"><div><br></div></div></div><span class="m_6608578721823455276gmail-HOEnZb"><font color="#888888">-- <br><div class="m_6608578721823455276gmail-m_-5044091989478664758gmail_signature">Breno Rodrigues Guimarães<div>Universidade Federal de Minas Gerais - UFMG, Brasil</div><div>(Federal University of Minas Gerais, Brazil)</div><div><br></div><div><br></div></div>
</font></span></div>
<br>______________________________<wbr>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div></div></div><br></div></div>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature" data-smartmail="gmail_signature">Breno Rodrigues Guimarães<div>Universidade Federal de Minas Gerais - UFMG, Brasil</div><div>(Federal University of Minas Gerais, Brazil)</div><div><br></div><div><br></div></div>
</div>