[PATCH] D131319: [clang-tidy] Update llvm-prefer-isa-or-dyn-cast-in-conditionals with new syntax

[Aman LaChapelle via Phabricator via cfe-commits]

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

This is great, thank you for doing this! I'm not a competent reviewer for the 
actual clang-tidy code changes but the +1 for the idea :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131319

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

[PATCH] D131319: [clang-tidy] Update llvm-prefer-isa-or-dyn-cast-in-conditionals with new syntax

[Aman LaChapelle via Phabricator via cfe-commits]

bzcheeseman added a comment.

In D131319#3709671 , @whisperity 
wrote:

> In D131319#3708667 , @bzcheeseman 
> wrote:
>
>> This is great, thank you for doing this! I'm not a competent reviewer for 
>> the actual clang-tidy code changes but the +1 for the idea :)
>
> The problem with the approval here is that a single approval will turn the 
> patch into a fully approved state, which breaks the dashboards for people 
> added to the patch (i.e., other reviewers will think the patch is already 
> approved, and thus perhaps will not consider putting effort into reviewing 
> it!).
>
> However, I think you should try the //Award Token// option from the menu on 
> the right! Somewhere the awarded tokens should show up on the patch, tallying 
> up support!

Ah I had no idea, thanks for pointing that out. I was looking at the Code 
Review document (https://llvm.org/docs/CodeReview.html), I'll put up a patch to 
add a short section on this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131319

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

[PATCH] D125709: [analyzer][Casting] Support isa, cast, dyn_cast of SVals

[Aman LaChapelle via Phabricator via cfe-commits]

bzcheeseman added inline comments.



Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h:102
   template
   T castAs() const {
 assert(T::classof(*this));

Is it worth refactoring this and getAs to use `cast`  and `dyn_cast` as 
appropriate to avoid code duplication?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125709

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

[PATCH] D125709: [analyzer][Casting] Support isa, cast, dyn_cast of SVals

[Aman LaChapelle via Phabricator via cfe-commits]

bzcheeseman added a comment.

In D125709#3518096 , @steakhal wrote:

> I had to fix the `doCast` to return `To` instead of `Optional` to make it 
> work.

That's fine (or it should be!) but you can also just dereference the optional, 
right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125709

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

[PATCH] D125709: [analyzer][Casting] Support isa, cast, dyn_cast of SVals

[Aman LaChapelle via Phabricator via cfe-commits]

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

> In D125709#3519033 , @bzcheeseman 
> wrote:
>
>> In D125709#3518096 , @steakhal 
>> wrote:
>>
>>> I had to fix the `doCast` to return `To` instead of `Optional` to make 
>>> it work.
>>
>> That's fine (or it should be!), you could dereference the optional if you 
>> wanted to
>
> Currently, we expect that casts result in regular SVal objects, instead of 
> pointer-like objects , thus this code to compile:
> `NonLoc N = llvm::cast(V)`, where `V` is of type `SVal`. I believe 
> that is why I decided to make that change.

Makes sense, LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125709

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

[PATCH] D125749: [analyzer][NFC] Introduce SVal::isa

[Aman LaChapelle via Phabricator via cfe-commits]

bzcheeseman added a comment.

Not a qualified reviewer for anything other than the `llvm::isa` usage and that 
line looks good to me!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125749

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

[PATCH] D126803: [llvm][analyzer][NFC] Introduce SFINAE for specializing FoldingSetTraits

[Aman LaChapelle via Phabricator via cfe-commits]

bzcheeseman added inline comments.



Comment at: llvm/include/llvm/ADT/FoldingSet.h:260
 /// to FoldingSets that were not originally designed to have that behavior.
-template struct FoldingSetTrait
-  : public DefaultFoldingSetTrait {};
+template 
+struct FoldingSetTrait : public DefaultFoldingSetTrait {};

supernit: I generally prefer named template parameters - can call it something 
like `Enable`?



Comment at: llvm/include/llvm/ADT/FoldingSet.h:834
+struct FoldingSetTrait<
+T, typename std::enable_if_t::value, void>> {
+  static void Profile(const T &X, FoldingSetNodeID &ID) {

supernit - the default type of std::enable_if_t is `void` so you don't need to 
specify it :)


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

https://reviews.llvm.org/D126803

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