[PATCH] D114034: [clang-tidy] fix debug-only test failure

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added a comment.

In D114034#3141446 , @mattbeardsley 
wrote:

> @dstenb Thanks for the confirmation!
>
> @kbobyrev Sorry about my mixup! Would you be able to help me commit this?

Done! Thank you for the patch!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114034

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

[PATCH] D114034: [clang-tidy] fix debug-only test failure

[Kirill Bobyrev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG85e03cb7ebac: [clang-tidy] fix debug-only test failure 
(authored by mattbeardsley, committed by kbobyrev).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114034

Files:
  clang-tools-extra/test/clang-tidy/infrastructure/pr37091.cpp


Index: clang-tools-extra/test/clang-tidy/infrastructure/pr37091.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/pr37091.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/pr37091.cpp
@@ -6,5 +6,5 @@
 //
 // Verify that no temporary files are left behind by the clang-tidy invocation.
 
-// RUN: env TMPDIR=%t TEMP=%t TMP=%t clang-tidy %s -- --target=mips64
+// RUN: env TMPDIR=%t TEMP=%t TMP=%t clang-tidy 
-checks='-*,clang-analyzer-core.NullDereference' %s -- --target=mips64
 // RUN: rmdir %t


Index: clang-tools-extra/test/clang-tidy/infrastructure/pr37091.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/pr37091.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/pr37091.cpp
@@ -6,5 +6,5 @@
 //
 // Verify that no temporary files are left behind by the clang-tidy invocation.
 
-// RUN: env TMPDIR=%t TEMP=%t TMP=%t clang-tidy %s -- --target=mips64
+// RUN: env TMPDIR=%t TEMP=%t TMP=%t clang-tidy -checks='-*,clang-analyzer-core.NullDereference' %s -- --target=mips64
 // RUN: rmdir %t
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114034: [clang-tidy] fix debug-only test failure

[Matt Beardsley via Phabricator via cfe-commits]

mattbeardsley added a comment.

@dstenb Thanks for the confirmation!

@kbobyrev Sorry about my mixup! Would you be able to help me commit this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114034

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

[PATCH] D114034: [clang-tidy] fix debug-only test failure

[Kirill Bobyrev via Phabricator via cfe-commits]

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

LG, thanks! I skimmed through the original patch and couldn't understand the 
`RenameFcn` part, thanks for the explanation!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114034

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

[PATCH] D114034: [clang-tidy] fix debug-only test failure

[Matt Beardsley via Phabricator via cfe-commits]

mattbeardsley updated this revision to Diff 388329.
mattbeardsley edited the summary of this revision.
mattbeardsley added a comment.

Well, I've goofed. I was working in a shared fork where someone added a 
RenameFcn check locally (not sure we should be doing that, but that's besides 
the point). I thought I was in my vanilla llvm-project fork when I saw the 
failure. So some of the earlier messages don't apply.

I still think it'd be good to isolate pr37091.cpp from the top-level 
.clang-tidy! But I've reverted the top-level .clang-tidy change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114034

Files:
  clang-tools-extra/test/clang-tidy/infrastructure/pr37091.cpp


Index: clang-tools-extra/test/clang-tidy/infrastructure/pr37091.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/pr37091.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/pr37091.cpp
@@ -6,5 +6,5 @@
 //
 // Verify that no temporary files are left behind by the clang-tidy invocation.
 
-// RUN: env TMPDIR=%t TEMP=%t TMP=%t clang-tidy %s -- --target=mips64
+// RUN: env TMPDIR=%t TEMP=%t TMP=%t clang-tidy 
-checks='-*,clang-analyzer-core.NullDereference' %s -- --target=mips64
 // RUN: rmdir %t


Index: clang-tools-extra/test/clang-tidy/infrastructure/pr37091.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/pr37091.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/pr37091.cpp
@@ -6,5 +6,5 @@
 //
 // Verify that no temporary files are left behind by the clang-tidy invocation.
 
-// RUN: env TMPDIR=%t TEMP=%t TMP=%t clang-tidy %s -- --target=mips64
+// RUN: env TMPDIR=%t TEMP=%t TMP=%t clang-tidy -checks='-*,clang-analyzer-core.NullDereference' %s -- --target=mips64
 // RUN: rmdir %t
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114034: [clang-tidy] fix debug-only test failure

[David Stenberg via Phabricator via cfe-commits]

dstenb added a comment.

In D114034#3136057 , @mattbeardsley 
wrote:

> @dstenb - wanted to check with you too to make sure that this change to 
> pr37091.cpp seems like it won't interfere with the original intent of the 
> test (https://reviews.llvm.org/D45686 )
>
> The lingering file issue you'd fixed seemed to be independent of any 
> particular clang-tidy check, but if not, hopefully it can at least get away 
> with not relying on the top-level .clang-tidy anymore!

Thanks for asking!

Yes, I think that the issue was seen independently of the checks used. We saw 
that issue originally with clang-tidy invocations with explicit -checks 
arguments (specifying clang-analyzer-* and some more checks). So, I guess that 
just specifying some check here should be fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114034

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

[PATCH] D114034: [clang-tidy] fix debug-only test failure

[Matt Beardsley via Phabricator via cfe-commits]

mattbeardsley added a subscriber: dstenb.
mattbeardsley added a comment.

@dstenb - wanted to check with you too to make sure that this change to 
pr37091.cpp seems like it won't interfere with the original intent of the test 
(https://reviews.llvm.org/D45686 )

The lingering file issue you'd fixed seemed to be independent of any particular 
clang-tidy check, but if not, hopefully it can at least get away with not 
relying on the top-level .clang-tidy anymore!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114034

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

[PATCH] D114034: [clang-tidy] fix debug-only test failure

[Matt Beardsley via Phabricator via cfe-commits]

mattbeardsley created this revision.
mattbeardsley added a reviewer: kbobyrev.
Herald added a subscriber: xazax.hun.
mattbeardsley requested review of this revision.
Herald added subscribers: cfe-commits, aheejin.
Herald added a project: clang-tools-extra.

The clang-tidy/infrastructure/pr37091.cpp test inherits the top-level 
.clang-tidy configuration because it doesn't specify its own checks, which ends 
up including misc-RenameFcn. This results in an assert when built with 
"-DCMAKE_BUILD_TYPE=Debug" because fQualifiedName is an empty string in 
"hasName(fQualifiedName)" in RenamefcnCheck.cpp (because none of the config 
options for misc-RenameFcn are provided - not that they should be). The 
HasNameMatcher::HasNameMatcher implementation behind hasName(...) asserts on 
empty strings.

Fixed in 2 ways:

- make the clang-tidy/infrastructure/pr37091.cpp test independent of the 
top-level .clang-tidy (picked an arbitrary check that I saw another 
clang-tidy/infrastructure test was also using: 
clang-tidy/infrastructure/temporaries.cpp)
- It's not useful for the top-level .clang-tidy to have misc-RenameFcn enabled 
anyway, so disabled that

(each of those changes fixes the debug-only assert in 
clang-tidy/infrastructure/pr37091.cpp on its own, but I thought each change had 
its own merits)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D114034

Files:
  .clang-tidy
  clang-tools-extra/test/clang-tidy/infrastructure/pr37091.cpp


Index: clang-tools-extra/test/clang-tidy/infrastructure/pr37091.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/pr37091.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/pr37091.cpp
@@ -6,5 +6,5 @@
 //
 // Verify that no temporary files are left behind by the clang-tidy invocation.
 
-// RUN: env TMPDIR=%t TEMP=%t TMP=%t clang-tidy %s -- --target=mips64
+// RUN: env TMPDIR=%t TEMP=%t TMP=%t clang-tidy 
-checks='-*,clang-analyzer-core.NullDereference' %s -- --target=mips64
 // RUN: rmdir %t
Index: .clang-tidy
===
--- .clang-tidy
+++ .clang-tidy
@@ -1,4 +1,4 @@
-Checks: 
'-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes,-misc-no-recursion,readability-identifier-naming'
+Checks: 
'-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes,-misc-no-recursion,-misc-RenameFcn,readability-identifier-naming'
 CheckOptions:
   - key: readability-identifier-naming.ClassCase
 value:   CamelCase


Index: clang-tools-extra/test/clang-tidy/infrastructure/pr37091.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/pr37091.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/pr37091.cpp
@@ -6,5 +6,5 @@
 //
 // Verify that no temporary files are left behind by the clang-tidy invocation.
 
-// RUN: env TMPDIR=%t TEMP=%t TMP=%t clang-tidy %s -- --target=mips64
+// RUN: env TMPDIR=%t TEMP=%t TMP=%t clang-tidy -checks='-*,clang-analyzer-core.NullDereference' %s -- --target=mips64
 // RUN: rmdir %t
Index: .clang-tidy
===
--- .clang-tidy
+++ .clang-tidy
@@ -1,4 +1,4 @@
-Checks: '-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes,-misc-no-recursion,readability-identifier-naming'
+Checks: '-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes,-misc-no-recursion,-misc-RenameFcn,readability-identifier-naming'
 CheckOptions:
   - key: readability-identifier-naming.ClassCase
 value:   CamelCase
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits