In D146678#4220360, @yeswalrus wrote:

-I can handle it. -
Edit: I cannot handle it, I don't have commit access. If you could commit to both main and the 16.x lines with the attribution to Walter Gray <yeswalrus@gmail.com> that would be much appreciated. Thanks!

While I'd definitely like to see the pre-commit bots finish before committing this, Aaron tells me there seems to be a series of problems (we had an outage earlier this week, we might be seeing more from it!). Feel free to commit without waiting, post-commit works, so they can catch anything that went wrong.

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

Test looks fine, still need a release note.

Just noticed your commit-notice reference to the 'breaking' change is incorrect, should be:
https://reviews.llvm.org/D139705

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

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

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.

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();
}

In D139705#4216480, @aaron.ballman wrote:

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

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?

Before the change, for both x and temp<double>, prior to the change the getSourceRange() on the VarDecl that represents them includes an initializer (they end just before ;). But now for the variable template specialization, we end up just after template arguments. This means that fixit/report that previously covered the whole definition, will now not include an initializer.
Or in our example:

template<typename T>
T temp = 1;
     // v getSourceRange() ends on this token before and after the change
int x = 10;
                              // v getSourceRange() ends on this token prior to the change, consistently with normal variables
template<> double temp<double> = 1;
                          // ^ getSourceRange() ends on this token after the change, making it inconsistent

Thank you for the further explanation, that clarified the concern for me as well. I think I agree with you -- we used to cover the full source range for the AST node, and now we only cover part of it because we're missing the initializer. We want getSourceRange() to cover the full range of text for the node, so we should have a different function to access the more limited range. @v1nh1shungry is this something you'd feel comfortable fixing up?

This generally looks good to me, but I get lost in the parser pretty quick, so hoping one of the other reviewers can take a look.

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.

Changes aaron suggested. Chose zip_longest instead of enumerate.

In D146426#4207118, @aaron.ballman wrote:

This feels like it's heading in the wrong direction -- the AST should not have holes in it. An invalid type should be replaced by a valid type (after diagnosing the invalid type, of course) so that we can keep as much of the AST around as possible (for example, we typically stub in int and continue compilation, as in: https://godbolt.org/z/MvxjGovGh), which should then result in a non-null ParmVarDecl. This way, we don't need to sprinkle nullptr checks all over the compiler when inspecting a function's parameters.

I've got the 1 concern with the mangling that I REALLY want one of our codegen owners to chime in on, otherwise this LGTM.

Added the code owners here. There doesn't seem to be anything questionable here to me, but I want to make sure someone who really knows what is going on takes a look.

Still LGTM.

To highlight the fix. See the rest of our release notes.

Please add a release note as requested.

In D146140#4201481, @cor3ntin wrote:

Do we need a release note for this?

Please add the test cases requested, plus a Release Note. Otherwise LGTM.

In D131307#4197767, @rsmith wrote:

Based on feedback we will provide users to the ability to downgrade this this diagnostic to a waring to allow for a transition period. We expect to turn this diagnostic to an error in the next release.

Can we revert this now?

Still LGTM!

In D146140#4196919, @rymiel wrote:

In D146140#4196886, @erichkeane wrote:

Ah, also, before commit, ensure that pre-commit CI passes.

Yep, found an issue already ;;

In D146140#4196866, @erichkeane wrote:

I presume you don't have commit access

I do! I just haven't done anything in this part of the monorepo, which is why i'm so lost.

Ah, also, before commit, ensure that pre-commit CI passes.

So I'm not a great person to review this, and I don't have a good idea what this is doing. I'm hopeful that @craig.topper can.

Thank you! This looks good to me. I presume you don't have commit access, so if you give me your "SomeName <EmailAddress>", I can commit this for you.

In D145851#4194154, @royjacobson wrote:

Fix the codegen test, add a standard ref to the comment.

In D145892#4193522, @HerrCai0907 wrote:

Hello Erichkeane, I have the access now, maybe I can push it by myself. Thanks again!

In D145892#4193428, @HerrCai0907 wrote:

I don't have write access now. Could you kindly help me to commit it?

Looks good! Thanks! Commit when you're ready.

LGTM with 2 nits.

In D145892#4193085, @HerrCai0907 wrote:

Should I add release note in clang/docs/ReleaseNotes.rst or any other way?

Thanks for your patience, I was buried trying to get the 16.0 release done. This LGTM!

In D145892#4191220, @HerrCai0907 wrote:

I cannot find an example that throw error: expression contains unexpanded parameter pack 'T' in fold expression because code like (foo<T>(10 + (static_cast<U>(1))) + ...); can be unpack both T and U in same ... operator

But this code can be diagnosed correctly

template <typename... U> struct A {
  template <typename... T> void foo(T &&...Params) {
    (foo<T>(1 + (static_cast<U>(1))) + ...); // ok

    // error: expression contains unexpanded parameter pack 'T'
    foo<T>((... + static_cast<U>(1)));

    (foo<T>((... + static_cast<U>(1))) + ...); // ok
  }
};

Should I add this case into test?

First, we've been dealing with the 16.0 release process/regressions, which unfortunately got us pretty far behind on reviews. Sorry about that!

In D145851#4189916, @royjacobson wrote:

In D145851#4189158, @erichkeane wrote:

Generally looks good to me. Do we do anything special if there are multiple initializers? Also, can we have a codegen test that validates that we actually construct it correctly (and perhaps a constexpr test for the same!)?

Added constexpr + codegen tests.

If we have multiple initializers it's an error that's already diagnosed, I assume the error recovery just drops any initializer after the first.

In D145892#4189603, @HerrCai0907 wrote:

The idea is that:

1. If function return an invalid ExprResult, this error should be handled in the callee
2. If function return valid Expr, the potential error should be handled in the caller, in this case, is ActOnExprStmt. If callee handles error, when caller find some error and want to output error message, the error in this expression has been processed and cause crash.

It seems this patch would make us miss other issues in the Foo<T>(Unknown) if they were to exist,

I try this code and it will not miss other issues

c++
template <typename... T>
void foo(T &&...Params) {
  Unknown;
  foo<T>(Unknown);
  ((foo<T>(Unknown + (10 / 0) + 10++)), ...);
}

build-demo/a.cpp:3:3: error: use of undeclared identifier 'Unknown'
  Unknown;
  ^
build-demo/a.cpp:4:3: error: expression contains unexpanded parameter pack 'T'
  foo<T>(Unknown);
  ^   ~
build-demo/a.cpp:5:35: error: expression is not assignable
  ((foo<T>(Unknown + (10 / 0) + 10++)), ...);
                                ~~^
build-demo/a.cpp:5:12: error: use of undeclared identifier 'Unknown'
  ((foo<T>(Unknown + (10 / 0) + 10++)), ...);
           ^
build-demo/a.cpp:5:26: warning: division by zero is undefined [-Wdivision-by-zero]
  ((foo<T>(Unknown + (10 / 0) + 10++)), ...);
                         ^ ~
build-demo/a.cpp:4:10: error: use of undeclared identifier 'Unknown'
  foo<T>(Unknown);
         ^
1 warning and 5 errors generated.

This needs a release note. Also, the patch message doesn't do a good job explaining what is going on here.

In D140996#4189452, @bolshakov-a wrote:

Sorry! It's my first time using Phabricator. Maybe, the problem occurs because I've solved the issue with Arcanist just by means of copy-pasting patches into "Update Diff" Web GUI form. Maybe, I should reopen the PR?

Patch seems to be missing all the context.

sorry for the delay, just started unburying myself from the 16.0 release process. Its unfortunate we don't have a great way to test this, but I can see the value of having it anyway in clangd.

Generally looks good to me. Do we do anything special if there are multiple initializers? Also, can we have a codegen test that validates that we actually construct it correctly (and perhaps a constexpr test for the same!)?

Thanks for the ping. I don't have any better ideas/other thoughts, so LGTM.

Patch topic needs "WIP" out of it I think?

So here's a potential idea for future development: It isn't uncommon/untypical for an attribute to want to return something other than '1', for 'version' (usually an integral value representing a date). The standard attributes all do this. It might be worth looking into some infrastructure to do that.

I want to wait for @aaron.ballman to answer, but i think this is generally OK. Note the motivation is the ability to check this from has_cpp_attribute/has_c_attribute/etc.

Looks like this doesn't compile pre-commit, though no idea if that is a patch-stack issue. Other than test, patch looks fine.

Looks like you have a clang-format fix to do (see pre-commit CI), else, LGTM.

clang-format fixes, going to make sure this passes CI, then commit

Add tests from regression bug report.

Going to add 1 more set of test beyond the revert, which is the tests from that regression report.

Patch itself seems fine enough, but I want to give others a chance to poke at it. The name makes me grumbly, but if there is sufficient 'prior art' here, I'm ok with it.

Other than the two auto's missing a *, I think this is alright. I want to think about it/give others a chance to review, so please ping this in a few days if it hasn't been accepted by then.

Not thrilled about 'mangled name' still, but I can't think of anything better after all this time, so LGTM.

This needs a release note. Otherwise I just have a preference for the 'is this spelling/syntax combo provided by this attribute' being in the plugin infrastructure. It DOES make me wonder however, what happens if TWO plugins provide a different 'true' answer for those?