Rebasing on main

Added function to query stack frame for temporary object.

Fixed formatting to match clang-format

Remove excessive reformating

Updated MoveChecker to handle CXXLifetimeExtendedObjects

In D151325#4374326, @NoQ wrote:

I totally support this! There's no reason why the region wouldn't carry such information.

This patch is surprisingly tiny, but this matches my grep observations: almost nothing in the static analyzer treats CXXTempObjectRegion specially. Even in case of live symbols/regions analysis, it simply relies on how temporary region is bound in the Store to the lifetime-extending reference, so it doesn't need to be handled explicitly.

In D146733#4219448, @erichkeane wrote:

LGTM! Let me know if you need someone to commit this for you, and include "Name To Use <EmailAddr>"

Rebasing on test commit. My applogies for noise, I still get confused with arc sometimes.

Reintroduced bug commit refernces. Sorry for the noise, but I still sometimes get confused with arc.

Reverted accidentally included changes.

Updated FIXME with GitHub bug reference.

This is now tracked as https://github.com/llvm/llvm-project/issues/61680. Let me know if I should refer it in the test.

Updated release note to be more descriptive.

Including release note

Added FIXME

Adding tests.

Merged indidictal check lines

While creating a test for the fix, I noticed the getSourceRange behavior is not stable after the serialization, as illustrated here: https://reviews.llvm.org/D146784

I do not plan to address the behavior in case of a variable without initializer. Creating separate PR to illustrate that is not a regression from my change.

I have updated the description and implementation. Will look at adding a unit test tomorrow.

Simpified condition

In D146733#4216931, @erichkeane wrote:

This needs a test and a release note. Patch otherwise looks fine to me.

Comments no longer misleading reffer to fallback to DeclatorDecl

Indeed, this would address our concern, and allow properly inserting initializer. This would build down to repeating the condition from here: https://github.com/llvm/llvm-project/blob/bbe1394c9602ab9a939d9b17199d5f538cac9d0c/clang/lib/AST/Decl.cpp#L1207.

I was suggesting adding an additional function, as it would cover additional use cases like replacing or removing the initializer, and then VarDecl::getSourceRange could be defined as:

SourceRange VarDecl::getSourceRange() const {
  if (const Expr *Init = getInit()) {
    SourceLocation InitEnd = Init->getEndLoc();
    // If Init is implicit, ignore its source range and fallback on
    // DeclaratorDecl::getSourceRange() to handle postfix elements.
    if (InitEnd.isValid() && InitEnd != getLocation())
      return SourceRange(getOuterLocStart(), InitEnd);
  }
  return getDeclatorRange();
}

That would make sense to me. Feel free to submit a patch (assuming @v1nh1shungry doesn't respond/get to it first).

Since the motivation for this patch here was "make sure we're pointing to the 'end' so we can suggest an init fixit", perhaps this SHOULD be changed to the 'end' still, but just fix the case where the initializer was omitted. So

/// What USED to happen:
template<> double temp<double> = 1;
//End is here:                   ^
template<> double temp<double>;
//End is here:       ^

//What is happening now:
template<> double temp<double> = 1;
//End is here:               ^
template<> double temp<double>;
//End is here:               ^

// What I think we want:
template<> double temp<double> = 1;
//End is here:                   ^
template<> double temp<double>;
//End is here:               ^

Right? So this isn't so much as a separate function, its just to make sure we get the 'source range' to include 'everything', including the initializer, if present.

In D139705#4216417, @erichkeane wrote:

In D139705#4215653, @tomasz-kaminski-sonarsource wrote:

As a downstream, we have concerns with this change. From what I saw it breaks the behavior of the fix-it that wants to remove the whole variable definition (including the initializer). For example, I have unused, that I want to remove variable x and unnecessary explicit specialization temp<double>:

template<typename T>
T temp = 1;

int x  = 10;
template<> double temp<double> = 1;

Previously, this could be handled (in case of single variable declaration) by simply removing up the var->getSourceRange() with succeeding coma. However, after the change, such code does not work, and in general if getSourceRange() is used on VarDecl (or even Decl), special consideration needs to be taken for the situation when VarDecl refers to variable template specialization.

As an alternative, I would suggest introducing an additional function to VarDecl (which could be moved up in the hierarchy), that would report a source range that corresponds to declarator-id, i.e. for template variable it would include template arguments.

Hmm... I'm being a little dense here I guess, I don't understand what you mean? Does this result in one of our fixits being wrong here? With the old range, wouldn't it have left the <double> in that case, and now is removing it? Or what am I missing?

As a downstream, we have concerns with this change. From what I saw it breaks the behavior of the fix-it that wants to remove the whole variable definition (including the initializer). For example, I have unused, that I want to remove variable x and unnecessary explicit specialization temp<double>:

template<typename T>
T temp = 1;

Added assert

Added assertion.

In D136671#3882166, @isuckatcs wrote:

This eliminate the crash in getDynamicElementCount on that region

I think that if the crash comes from getDynamicElementCount, we should address it there too, so
when the function is called elsewhere under the same circumstances it won't crash again.

What do you think?

As a result of our internal test on around ~170 projects (~20 Widnows, ~150 Linux) that are compromised of several hundreds of millions of lines of code, the impact on the files that parsed correctly was: 5 issues disappearing and 4 new issues. We investigated all of the reports, and the changes seemed justified:

Applied suggested comment updates.

Fighting with arcanist.

Updated diff to be mergable.

Applied all review suggestions.

Applied review suggestions.

Included additional tests that corresponds to TODO.

Added requested comment for the example.
Included additional false-positive test.

What looks fishy about getInterestingValues() is that it assumes that the amount of interesting values is finite. This sounds incredibly wrong to me. If a lazy compound value contains any pointer symbol $p, then all values in the following infinite series are interesting:

$p,  *$p,  **$p,  ***$p,  ...

Added new line

Ping and reformat.

• Pass BldrCtx to handleConstructionContext

Ping.

@NoQ Do you agree with my view on handleConstructionContext, if so is this patch ready to land?

Fixed formatting.