Thanks for working on this!

Friendly ping

In D88332#2298301, @baloghadamsoftware wrote:

I would even go further. Why Clang Static Analyzer? I see nothing path-sensitive here. IMHO a better place to implement this functionality is Clang-Tidy.

In D88332#2297374, @NoQ wrote:

Hmm, if we could add an interface to CallDescriptionMap to accept AnyCall instead of CallEvent then we could use it for path-insensitive checkers as well. That'd have saved some lines of code.

Sounds interesting but I lack context to imagine the details. Would the map be still initialized by some ASTVisitor? Do you think there'd be any noticeable performance hit?

Had to fix the order of expected warnings in warning-wall.c
6fd8c69049a8 [clang] Update warning-wall.c test

Friendly ping.
I'd like to land this sooner than later - I agree it'd be great to have a regression test but my (possibly biased) opinion is that the change is also simple and obvious enough that we could land it without a test.

Friendly ping

Friendly ping

Created this for eventual post-commit review.

I made a typo in the commit message reference so this revision didn't get closed automatically.

Thank you Artem!

Thank you both for all the hints!

Simply check memory regions of LHS and RHS in assignments instead of AST pattern matching.

Nits were addressed (hopefully).

Attempt to write a unit test - so far can't reproduce even with unpatched master :(

Added test

Friendly ping

Hi @usaxena95 and @sammccall,

• renamed
• added tests

Sorry to bother you @NoQ - it'd be awesome if you could take a look!

I forgot to add the magic "Differential revision:" prefix to the commit message :(

• fix the doc comment
• rename

• added tests
• renamed with prefix

I could be missing something but it seemed that skipping module maps when we're searching for source locations is obviously the right thing to do. Don't really know if repeating what the code does in the comment would bring any value.

Add comment

Unfortunately, I don't have a test, sorry. I'm not trying to cut corners here - just thought that iterator referring to begin() value of a container should never be decremented so given the nature of the issue, simplicity of the fix and expected effort necessary to find a reliable test case it'd be more productive if I spend my time doing other work.

You are absolutely right! I initially didn't realize the overloads of Rewriter methods can be of importance and haven't looked back since.

@akyrtzi IIUC you are saying we should try to push the char-range semantics from libclang API as deep as possible to see if we could avoid this annoying conversion. I think that's a reasonable long-term strategy.
But I also think we'll inevitably run into some clang APIs that use SourceRange for valid reasons and we won't be able to change those. Some other clang APIs might require significant amount of work to be enhanced.

I believe this is the correct approach: https://reviews.llvm.org/D86990

When we return source ranges from functions in libclang we use cxloc::translateSourceRange to convert them from SourceRange semantics to half-open character range CXSourceRange. I checked the cxloc::translateSourceRange implementation and also that we use it if not everywhere then very often.

In D86840#2248467, @akyrtzi wrote:

In D86840#2248463, @jkorous wrote:

Thanks a lot for this clarification! So, does that mean the issue is actually here?

static inline SourceRange translateCXSourceRange(CXSourceRange R) {
  return SourceRange(SourceLocation::getFromRawEncoding(R.begin_int_data),
                     SourceLocation::getFromRawEncoding(R.end_int_data));
}

I think it depends on what that CXSourceRange represents (how was it created).

In D86840#2248453, @akyrtzi wrote:

AFAIK SourceRange is supposed to always represent a token range (begin loc points to beginning of first token, end loc points to beginning of last token). For representing a character range, CharSourceRange should be used, though IMO its IsTokenRange member was a mistake, CharSourceRange should have only being used to represent half-open character-based range. This distinction is thankfully more clear on the Swift side.

So, to me it seems that the only problematic code-path is when I'm plumbing source range from libclang API (where I naturally need to convert from CXSourceRange to some internal type) to Rewriter as Rewriter::getRangeSize() makes an assumption about the input which is missing from its signature and documentation (and by proxy from other Rewriter methods).

Added a random test and a bunch of notes (to be nuked before anything is committed).

Honestly, the more I'm looking into this, the more puzzled I am - please assume that my previous conclusions weren't necessarily correct. I started adding notes to the code so I can keep track of my thoughts - I'll update the revision.

Also, another thought - this idea is really alpha for us too in the sense that we still haven't gotten very good feel for how invasive it is. That means that it's pretty likely that it'll change and I would not be surprised if we for example decide to drop the requirement for embedded block.
I am wondering whether it makes sense to invest into refactoring now. Do you think landing it as it is would still somehow fit under the alpha label or is it expected such checkers to live out of tree until they mature enough?

Ping

Ping

Friendly ping

Friendly ping

fix typo

• added docs

Addressed comments.

Friendly ping

Restore the patch. Previous update wasn't meant for this review - I confused the review IDs.

Rebased on recent master + https://reviews.llvm.org/D66854

Rebased on recent master + https://reviews.llvm.org/D66854

distinguish between raw-ptr & reference

I didn't figure out how to match the code snippet with the caret by using -verify. IIUC it supports only {error,warning,remark,note} diagnostics while the code snippet is neither of these.
I took inspiration from clang/test/Misc/caret-diags-multiline.cpp and just use FileCheck %s --strict-whitespace.