<div class="gmail_quote">On Thu, Apr 5, 2012 at 12:49 AM, Nico Weber <span dir="ltr"><<a href="mailto:thakis@chromium.org">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">
Now with a driver-based test instead (addresses Joerg's comment too). Better?<br></blockquote><div><br></div><div>Awesome. Nits:</div><div><br></div><div><div>--- test/Driver/fno-pic.c<span class="Apple-tab-span" style="white-space:pre">     </span>(revision 0)</div>
<div>+++ test/Driver/fno-pic.c<span class="Apple-tab-span" style="white-space:pre">     </span>(revision 0)</div><div>@@ -0,0 +1,5 @@</div><div>+// RUN: %clang -c %s -o %t.o -target i386-apple-darwin -### 2>&1 | FileCheck %s --check-prefix=PIC_ON_BY_DEFAULT</div>
<div>+// PIC_ON_BY_DEFAULT: "-pic-level" "1"</div><div>+</div><div>+// RUN: %clang -c %s -target i386-apple-darwin -### -fno-pic 2>&1 | FileCheck %s --check-prefix=FNO_PIC</div><div>+// FNO_PIC: "-mrelocation-model" "static"</div>
</div><div><br></div><div>why '-o %t.o' in the first, and not in the second? It shouldn't make a difference with -###, was just curious about the inconsistency.</div><div><br></div><div><div>--- lib/Driver/Tools.cpp<span class="Apple-tab-span" style="white-space:pre">    </span>(revision 154058)</div>
<div>+++ lib/Driver/Tools.cpp<span class="Apple-tab-span" style="white-space:pre">      </span>(working copy)</div><div>@@ -1471,7 +1471,11 @@</div><div>                      Args.hasArg(options::OPT_fPIE) ||</div><div>                      Args.hasArg(options::OPT_fpie));</div>
<div>   bool PICDisabled = (Args.hasArg(options::OPT_mkernel) ||</div><div>-                      Args.hasArg(options::OPT_static));</div><div>+                      Args.hasArg(options::OPT_static)) ||</div><div>+                      Args.hasArg(options::OPT_fno_PIC) ||</div>
</div><div><br></div><div>I'm not great at reading indentation in patch files, so this may not be quite right, but it looks like the indent is off for these to me... specifically, the indent is the same for OPT_static and for OPT_fno_PIC, when in fact, they have different groupings...</div>
<div><br></div><div>But actually, the different groupings are a no-op, as everything is || here. I think the parens only existed to make indentation nicer, so maybe just remove the second karen after OPT_static to before the ';'?</div>
<div><br></div><div><br></div><div>Anyways, LGTM to submit with these style nits cleaned up. Thanks!</div></div>