[PATCH] D85788: [Clang test] Update to allow passing extra default clang arguments in use_clang

[Gui Andrade via Phabricator via cfe-commits]

guiand abandoned this revision.
guiand added a comment.

We decided to go with a positive flag for enabling noundef, so I'm closing this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85788/new/

https://reviews.llvm.org/D85788

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85788: [Clang test] Update to allow passing extra default clang arguments in use_clang

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

In D85788#2449299 , @aqjune wrote:

> In D85788#2444136 , @guiand wrote:
>
>> IMO it's better to just one-and-done programatically add `-Xclang 
>> -disable-noundef-analysis` to all the tests' RUN lines. That way there are 
>> no hidden flags.
>
> If a script for this can be written and people who are working for the 
> downstream project are happy with the script, this is the best approach IMO. 
> It is clear and explicit.

Yeah, I also think this would be the best.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85788/new/

https://reviews.llvm.org/D85788

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85788: [Clang test] Update to allow passing extra default clang arguments in use_clang

[Juneyoung Lee via Phabricator via cfe-commits]

aqjune added a comment.

In D85788#2444136 , @guiand wrote:

> IMO it's better to just one-and-done programatically add `-Xclang 
> -disable-noundef-analysis` to all the tests' RUN lines. That way there are no 
> hidden flags.

If a script for this can be written and people who are working for the 
downstream project are happy with the script, this is the best approach IMO. It 
is clear and explicit.

In D85788#2441457 , @eugenis wrote:

>> This new substitution is going to be used for the noundef checks in D81678 
>>  only anyway.
>
> I'm not sure what you mean by this. The substitution is used ~380 times in 
> this patch alone.
> In the future, any attempt to use %clang -### or -%clang -cc1as will produce 
> the same unused argument warning, and it is very unclear that is can be fixed 
> with %clang_bin. Note that code search for "-disable-noundef-analysis" does 
> NOT lead you to the definition of %clang_bin!

I see, I was naively thinking the `%clang` -> `%clang_bin` changes were here to 
simply show that they are equivalent if function signatures are not checked. I 
thought `%clang` could be still used for those tests.
But they were here because the upcoming patch will otherwise make them raise 
unused argument warning.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85788/new/

https://reviews.llvm.org/D85788

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85788: [Clang test] Update to allow passing extra default clang arguments in use_clang

[Gui Andrade via Phabricator via cfe-commits]

guiand added a comment.

This whole thing is a little unfortunate, but maybe a better substitution would 
be leaving `%clang` as referring to the pure clang binary, with default 
arguments. Then we can have a `%clang_cc` which may only be used for a standard 
C compiler invocation.

Returning to the main issue, adding a default argument to `%clang` breaks a 
number of tests. Generally these tests change the compiler mode such that a C 
compiler argument makes no sense. Consider `%clang -cc1`: this gets expanded to 
`clang -Xclang -disable-noundef-analysis -cc1`, which IIRC causes an error.
This isn't a problem with something like `%clang_cc`, because a C compiler flag 
would always be valid for that expansion.

IMO it's better to just one-and-done programatically add `-Xclang 
-disable-noundef-analysis` to all the tests' RUN lines. That way there are no 
hidden flags.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85788/new/

https://reviews.llvm.org/D85788

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85788: [Clang test] Update to allow passing extra default clang arguments in use_clang

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis requested changes to this revision.
eugenis added a comment.
This revision now requires changes to proceed.

I wonder if we could just disable unused argument warning for both

  -disable-noundef-analysis
  -Xclang -disable-noundef-analysis

I don't see any precedents for this kind of thing in the code.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85788/new/

https://reviews.llvm.org/D85788

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85788: [Clang test] Update to allow passing extra default clang arguments in use_clang

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

> simply naming it as %clang_noundef would be clearer,

Hmm, yeah, that's a little better.

> This new substitution is going to be used for the noundef checks in D81678 
>  only anyway.

I'm not sure what you mean by this. The substitution is used ~380 times in this 
patch alone.
In the future, any attempt to use %clang -### or -%clang -cc1as will produce 
the same unused argument warning, and it is very unclear that is can be fixed 
with %clang_bin. Note that code search for "-disable-noundef-analysis" does NOT 
lead you to the definition of %clang_bin!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85788/new/

https://reviews.llvm.org/D85788

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85788: [Clang test] Update to allow passing extra default clang arguments in use_clang

[Juneyoung Lee via Phabricator via cfe-commits]

aqjune added a comment.
Herald added a subscriber: frasercrmck.

In D85788#2335838 , @eugenis wrote:

> I wonder it can be avoided by
>
> - disable noundef analysis by default in cc1
> - always add -enable-noundef-analysis in the driver when invoking cc1

Would this cause more tests that use `%clang` to be updated, such as  
`test/CodeGen/mips-vector-return.c`?
IIUC, the tests in this patch are okay to have their `%clang` replaced with 
`%clang_bin` because they are not checking function signatures.

> I don't like the %clang_bin substitution - imho it's unclear for the test 
> authors when to use it instead of %clang, but I can't come up with a better 
> idea.

From a user's perspective, simply naming it as `%clang_noundef` would be 
clearer, IMO.
This new substitution is going to be used for the noundef checks in D81678 
 only anyway.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85788/new/

https://reviews.llvm.org/D85788

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85788: [Clang test] Update to allow passing extra default clang arguments in use_clang

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

I don't like the %clang_bin substitution - imho it's unclear for the test 
authors when to use it instead of %clang, but I can't come up with a better 
idea.

Is it true that %clang_bin is only necessary when the automatically added 
-disable-noundef-analysis flag triggers an unused argument warning, or are 
there other reasons?

I wonder it can be avoided by

- disable noundef analysis by default in cc1
- always add -enable-noundef-analysis in the driver when invoking cc1


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85788/new/

https://reviews.llvm.org/D85788

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85788: [Clang test] Update to allow passing extra default clang arguments in use_clang

[Gui Andrade via Phabricator via cfe-commits]

guiand added a comment.

Did we decide that we wanted this change then? I remember there being 
discussion around whether it's the right approach.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85788/new/

https://reviews.llvm.org/D85788

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85788: [Clang test] Update to allow passing extra default clang arguments in use_clang

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis accepted this revision.
eugenis added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85788/new/

https://reviews.llvm.org/D85788

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits