[cfe-commits] [patch] Honor -fno-pic, -fno-PIC, -fno-pie, -fno-PIE

[Nico Weber]

On Wed, Apr 4, 2012 at 3:56 PM, Chandler Carruth <chandlerc at google.com> wrote:
> On Thu, Apr 5, 2012 at 12:49 AM, Nico Weber <thakis at chromium.org> wrote:
>>
>> Now with a driver-based test instead (addresses Joerg's comment too).
>> Better?
>
>
> Awesome. Nits:
>
> --- test/Driver/fno-pic.c (revision 0)
> +++ test/Driver/fno-pic.c (revision 0)
> @@ -0,0 +1,5 @@
> +// RUN: %clang -c %s -o %t.o -target i386-apple-darwin -### 2>&1 |
> FileCheck %s --check-prefix=PIC_ON_BY_DEFAULT
> +// PIC_ON_BY_DEFAULT: "-pic-level" "1"
> +
> +// RUN: %clang -c %s -target i386-apple-darwin -### -fno-pic 2>&1 |
> FileCheck %s --check-prefix=FNO_PIC
> +// FNO_PIC: "-mrelocation-model" "static"
>
> 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.
>
> --- lib/Driver/Tools.cpp (revision 154058)
> +++ lib/Driver/Tools.cpp (working copy)
> @@ -1471,7 +1471,11 @@
>                       Args.hasArg(options::OPT_fPIE) ||
>                       Args.hasArg(options::OPT_fpie));
>    bool PICDisabled = (Args.hasArg(options::OPT_mkernel) ||
> -                      Args.hasArg(options::OPT_static));
> +                      Args.hasArg(options::OPT_static)) ||
> +                      Args.hasArg(options::OPT_fno_PIC) ||
>
> 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...
>
> 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 ';'?
>
>
> Anyways, LGTM to submit with these style nits cleaned up. Thanks!

All done, landed in r154064. Thanks!