[PATCH] D58178: [clang-tidy] RawStringLiteralCheck: isRawStringLiteral doesn't check all erroneous cases

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D58178#1405430 , @gmit wrote:

> In D58178#1405397 , @MyDeveloperDay 
> wrote:
>
> > Could I ask if you don't want to pursue this review that you Abandon it.
>
>
> Sure, I already wanted that, but I don't see any options that would result in 
> closing the issue. Could you, please, point me to it? Tnx!


Scroll to the bottom of the review to the "Add Action..." combo box, then 
select "Abandon Revision"

F8324493: image.png 


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58178



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

[PATCH] D58178: [clang-tidy] RawStringLiteralCheck: isRawStringLiteral doesn't check all erroneous cases

[Goran Mitrovic via Phabricator via cfe-commits]

gmit added a comment.

In D58178#1405397 , @MyDeveloperDay 
wrote:

> Could I ask if you don't want to pursue this review that you Abandon it.


Sure, I already wanted that, but I don't see any options that would result in 
closing the issue. Could you, please, point me to it? Tnx!


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58178



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

[PATCH] D58178: [clang-tidy] RawStringLiteralCheck: isRawStringLiteral doesn't check all erroneous cases

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

Could I ask if you don't want to pursue this review that you Abandon it.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58178



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

[PATCH] D58178: [clang-tidy] RawStringLiteralCheck: isRawStringLiteral doesn't check all erroneous cases

[Goran Mitrovic via Phabricator via cfe-commits]

gmit added a comment.

In D58178#1396523 , @MyDeveloperDay 
wrote:

> I tried to repoduce this and I can't see how I could make a string literal 
> without a double quote, if it still failed with the current code it might 
> suggest the matcher matched something else.. understanding what that is is 
> probably more important, than just masking the failure.


I agree to that. There is not obvious reason why this would happen.

> there was a crash D19331: [Clang-tidy] Fix for crash in 
> modernize-raw-string-literal check  which 
> was fixed back in April, are you able to tell us the version/revision of 
> clang-tidy where this occurred

The crash happened on Clang 5 and I can confirm fix 19331 was present in the 
source files that built it.

> It's quite easy to trim down the source that crashes down to something usable 
> with https://embed.cs.utah.edu/creduce/
>  The end result won't resemble anything the original code was, no proprietary 
> secrets will be leaked.
>  Without test case, there is nothing do to here.

No, it's not that - I simply don't have it. I don't have .dmp either. I only 
have our fix that was done in our local Clang repository copy two years ago and 
a call stack.

Anyhow, I agree to dismiss this case. Sorry for wasting your time! Thanks!


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58178



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

[PATCH] D58178: [clang-tidy] RawStringLiteralCheck: isRawStringLiteral doesn't check all erroneous cases

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D58178#1396435 , @gmit wrote:

> ? I'm sorry, but I disagree.
>
> Assert draws attention in the debug build only.
>
> In the release build asserts are not evaluated at all and the condition needs 
> to be checked in the code that executes (if it makes sense and in this case 
> it does as it causes reading out of string bounds).


What i'm saying is, do you agree that the `assert(QuotePos != 
StringRef::npos);` happens before `return (QuotePos != StringRef::npos) && 
` ?
If you build with asserts enabled, that your original code which caused it to 
"read from memory randomly"
will trigger that assertion. Even with this fix. So the fix is not correct.

> I don't have a test as I don't have an input file that causes this behaviour.

It's quite easy to trim down the source that crashes down to something usable 
with https://embed.cs.utah.edu/creduce/
The end result won't resemble anything the original code was, no proprietary 
secrets will be leaked.
Without test case, there is nothing do to here.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58178



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

[PATCH] D58178: [clang-tidy] RawStringLiteralCheck: isRawStringLiteral doesn't check all erroneous cases

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

Rather than sending a review, probably the reproducible case as to why it crash 
is more important, it might be better to write up your bug at 
https://bugs.llvm.org/

I tried to repoduce this and I can't see how I could make a string literal 
without a double quote, if it still failed with the current code it might 
suggest the matcher matched something else.. understanding what that is is 
probably more important, than just masking the failure.

there was a crash D19331: [Clang-tidy] Fix for crash in 
modernize-raw-string-literal check  which was 
fixed back in April, are you able to tell us the version/revision of clang-tidy 
where this occurred


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58178



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

[PATCH] D58178: [clang-tidy] RawStringLiteralCheck: isRawStringLiteral doesn't check all erroneous cases

[Goran Mitrovic via Phabricator via cfe-commits]

gmit added a comment.

? I'm sorry, but I disagree.

Assert draws attention in the debug build only.

In the release build asserts are not evaluated at all and the condition needs 
to be checked in the code that executes (if it makes sense and in this case it 
does as it causes reading out of string bounds).

I don't have a test as I don't have an input file that causes this behaviour.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58178



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

[PATCH] D58178: [clang-tidy] RawStringLiteralCheck: isRawStringLiteral doesn't check all erroneous cases

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D58178#1396388 , @gmit wrote:

> The added condition should be obvious as it is tested in the assert just 
> above.


Well, then the fix is not correct. Because you won't get to that `return`,
since the `assert()` just above would have failed already..
This really needs a test.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58178



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