Page MenuHomePhabricator

erichkeane (Erich Keane)
User

Projects

User does not belong to any projects.

User Details

User Since
Jun 28 2016, 8:37 AM (351 w, 5 d)

Recent Activity

Fri, Mar 24

erichkeane added a comment to D146678: Fix unexpected nullptr in ConceptSpecializationExpr's ArgsAsWritten field.

-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!

Fri, Mar 24, 11:49 AM · Restricted Project, Restricted Project
erichkeane added a comment to D146678: Fix unexpected nullptr in ConceptSpecializationExpr's ArgsAsWritten field.

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.

Fri, Mar 24, 9:44 AM · Restricted Project, Restricted Project
erichkeane accepted D146733: [clang] source range of variable template specialization should include initializer.

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

Fri, Mar 24, 7:11 AM · Restricted Project, Restricted Project
erichkeane added inline comments to D141497: [clang][Interp] Record initialization via conditional operator.
Fri, Mar 24, 6:58 AM · Restricted Project, Restricted Project
erichkeane added inline comments to D146733: [clang] source range of variable template specialization should include initializer.
Fri, Mar 24, 6:56 AM · Restricted Project, Restricted Project
erichkeane added inline comments to D146595: [clang] Add clang trampoline_mangled_target attribute.
Fri, Mar 24, 6:37 AM · debug-info, Restricted Project, Restricted Project, Restricted Project
erichkeane added inline comments to D146784: [clang] Test for AST Dumping of the template variables.
Fri, Mar 24, 6:21 AM · Restricted Project, Restricted Project
erichkeane added a comment to D146733: [clang] source range of variable template specialization should include initializer.

Test looks fine, still need a release note.

Fri, Mar 24, 6:21 AM · Restricted Project, Restricted Project

Thu, Mar 23

erichkeane added a comment to D146733: [clang] source range of variable template specialization should include initializer.

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

Thu, Mar 23, 9:52 AM · Restricted Project, Restricted Project
erichkeane added inline comments to D146733: [clang] source range of variable template specialization should include initializer.
Thu, Mar 23, 9:48 AM · Restricted Project, Restricted Project
erichkeane added a comment to D146733: [clang] source range of variable template specialization should include initializer.

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

Thu, Mar 23, 9:45 AM · Restricted Project, Restricted Project
erichkeane added a comment to D139705: [clang] fix zero-initialization fix-it for variable template.

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();
}
Thu, Mar 23, 7:02 AM · Restricted Project, Restricted Project
erichkeane added a comment to D139705: [clang] fix zero-initialization fix-it for variable template.

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?

Thu, Mar 23, 6:50 AM · Restricted Project, Restricted Project
erichkeane added a comment to D146719: [Clang] Improve diagnostics when using a concept as template argument.

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.

Thu, Mar 23, 6:40 AM · Restricted Project, Restricted Project
erichkeane added a comment to D139705: [clang] fix zero-initialization fix-it for variable template.

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.

Thu, Mar 23, 6:17 AM · Restricted Project, Restricted Project
erichkeane accepted D146680: Fix unexpected nullptr in ConceptSpecializationExpr's ArgsAsWritten field.
Thu, Mar 23, 6:00 AM · Restricted Project, Restricted Project
erichkeane accepted D146678: Fix unexpected nullptr in ConceptSpecializationExpr's ArgsAsWritten field.
Thu, Mar 23, 5:59 AM · Restricted Project, Restricted Project

Tue, Mar 21

erichkeane committed rG67852bff5882: Fix switch warning from 514e4359a (authored by erichkeane).
Fix switch warning from 514e4359a
Tue, Mar 21, 8:52 AM · Restricted Project, Restricted Project
erichkeane committed rG01d05bd407bb: Add warning test to make buildbots happy after 514e4359 (authored by erichkeane).
Add warning test to make buildbots happy after 514e4359
Tue, Mar 21, 8:35 AM · Restricted Project, Restricted Project
erichkeane committed rG514e4359a543: inline stmt attribute diagnosing in templates (authored by erichkeane).
inline stmt attribute diagnosing in templates
Tue, Mar 21, 8:17 AM · Restricted Project, Restricted Project
erichkeane closed D146323: inline stmt attribute diagnosing in templates.
Tue, Mar 21, 8:17 AM · Restricted Project, Restricted Project
erichkeane added inline comments to D146323: inline stmt attribute diagnosing in templates.
Tue, Mar 21, 6:09 AM · Restricted Project, Restricted Project

Mon, Mar 20

erichkeane added inline comments to D146323: inline stmt attribute diagnosing in templates.
Mon, Mar 20, 12:45 PM · Restricted Project, Restricted Project
erichkeane updated the diff for D146323: inline stmt attribute diagnosing in templates.

Changes aaron suggested. Chose zip_longest instead of enumerate.

Mon, Mar 20, 12:18 PM · Restricted Project, Restricted Project
erichkeane added a comment to D146426: [Sema] Fix crash on __fp16 parameters in template instantiations.

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.

Mon, Mar 20, 12:07 PM · Restricted Project, Restricted Project
erichkeane added inline comments to D146420: Document the Clang policies on claiming support for a feature.
Mon, Mar 20, 7:14 AM · Restricted Project, Restricted Project
erichkeane updated subscribers of D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type..

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.

Mon, Mar 20, 7:08 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
erichkeane added a comment to D146386: [MS ABI] Fix mangling references to declarations..

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.

Mon, Mar 20, 6:13 AM · Restricted Project, Restricted Project
erichkeane added reviewers for D146386: [MS ABI] Fix mangling references to declarations.: eli.friedman, asl, rjmccall.
Mon, Mar 20, 6:12 AM · Restricted Project, Restricted Project

Fri, Mar 17

erichkeane requested review of D146323: inline stmt attribute diagnosing in templates.
Fri, Mar 17, 12:10 PM · Restricted Project, Restricted Project
erichkeane added a comment to D146140: [clang] Properly parse variable template requires clause in lambda.

Still LGTM.

Fri, Mar 17, 9:56 AM · Restricted Project, Restricted Project
erichkeane added a comment to D146089: [Sema] Fix null pointer dereference handleAlwaysInlineAttr..

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

Fri, Mar 17, 8:45 AM · Restricted Project, Restricted Project
erichkeane added a comment to D146089: [Sema] Fix null pointer dereference handleAlwaysInlineAttr..

Please add a release note as requested.

Fri, Mar 17, 6:21 AM · Restricted Project, Restricted Project
erichkeane added a comment to D146140: [clang] Properly parse variable template requires clause in lambda.

Do we need a release note for this?

Fri, Mar 17, 5:49 AM · Restricted Project, Restricted Project

Thu, Mar 16

erichkeane accepted D146089: [Sema] Fix null pointer dereference handleAlwaysInlineAttr..

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

Thu, Mar 16, 10:13 AM · Restricted Project, Restricted Project
erichkeane added inline comments to D146089: [Sema] Fix null pointer dereference handleAlwaysInlineAttr..
Thu, Mar 16, 5:49 AM · Restricted Project, Restricted Project

Wed, Mar 15

erichkeane added inline comments to D146089: [Sema] Fix null pointer dereference handleAlwaysInlineAttr..
Wed, Mar 15, 6:16 PM · Restricted Project, Restricted Project
erichkeane added a comment to D131307: [Clang] Allow downgrading to a warning the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values.

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?

Wed, Mar 15, 3:14 PM · Restricted Project, Restricted Project
erichkeane accepted D145851: [Clang][Sema] Fix incorrect deletion of default constructors for some unions.

Still LGTM!

Wed, Mar 15, 2:19 PM · Restricted Project, Restricted Project
erichkeane added inline comments to D146089: [Sema] Fix null pointer dereference handleAlwaysInlineAttr..
Wed, Mar 15, 12:37 PM · Restricted Project, Restricted Project
erichkeane added inline comments to D146089: [Sema] Fix null pointer dereference handleAlwaysInlineAttr..
Wed, Mar 15, 12:21 PM · Restricted Project, Restricted Project
erichkeane added inline comments to D146089: [Sema] Fix null pointer dereference handleAlwaysInlineAttr..
Wed, Mar 15, 11:01 AM · Restricted Project, Restricted Project
erichkeane added a comment to D146140: [clang] Properly parse variable template requires clause in lambda.

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

Yep, found an issue already ;;

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.

Wed, Mar 15, 9:33 AM · Restricted Project, Restricted Project
erichkeane added a comment to D146140: [clang] Properly parse variable template requires clause in lambda.

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

Wed, Mar 15, 9:22 AM · Restricted Project, Restricted Project
erichkeane added a comment to D146147: llvm/test/TableGen/intrinsic-*.td: Use Intrinsics.td instead of mock.

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.

Wed, Mar 15, 9:18 AM · Restricted Project, Restricted Project
erichkeane accepted D146140: [clang] Properly parse variable template requires clause in lambda.

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.

Wed, Mar 15, 9:16 AM · Restricted Project, Restricted Project
erichkeane added inline comments to D146140: [clang] Properly parse variable template requires clause in lambda.
Wed, Mar 15, 8:39 AM · Restricted Project, Restricted Project
erichkeane added inline comments to D146140: [clang] Properly parse variable template requires clause in lambda.
Wed, Mar 15, 8:17 AM · Restricted Project, Restricted Project
erichkeane added inline comments to D146140: [clang] Properly parse variable template requires clause in lambda.
Wed, Mar 15, 7:55 AM · Restricted Project, Restricted Project
erichkeane added inline comments to D146089: [Sema] Fix null pointer dereference handleAlwaysInlineAttr..
Wed, Mar 15, 5:59 AM · Restricted Project, Restricted Project

Tue, Mar 14

erichkeane accepted D145851: [Clang][Sema] Fix incorrect deletion of default constructors for some unions.

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

Tue, Mar 14, 12:32 PM · Restricted Project, Restricted Project
erichkeane added inline comments to D145851: [Clang][Sema] Fix incorrect deletion of default constructors for some unions.
Tue, Mar 14, 12:30 PM · Restricted Project, Restricted Project
erichkeane added a comment to D145892: [SemaCXX]use CorrectDelayedTyposInExpr in ActOnCXXFoldExpr only when Diag.

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

Tue, Mar 14, 9:48 AM · Restricted Project, Restricted Project
erichkeane added a comment to D145892: [SemaCXX]use CorrectDelayedTyposInExpr in ActOnCXXFoldExpr only when Diag.

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

Tue, Mar 14, 9:18 AM · Restricted Project, Restricted Project
erichkeane added a comment to D145892: [SemaCXX]use CorrectDelayedTyposInExpr in ActOnCXXFoldExpr only when Diag.

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

Tue, Mar 14, 9:03 AM · Restricted Project, Restricted Project
erichkeane accepted D145892: [SemaCXX]use CorrectDelayedTyposInExpr in ActOnCXXFoldExpr only when Diag.

LGTM with 2 nits.

Tue, Mar 14, 8:25 AM · Restricted Project, Restricted Project
erichkeane added inline comments to D145892: [SemaCXX]use CorrectDelayedTyposInExpr in ActOnCXXFoldExpr only when Diag.
Tue, Mar 14, 8:11 AM · Restricted Project, Restricted Project
erichkeane added a comment to D145892: [SemaCXX]use CorrectDelayedTyposInExpr in ActOnCXXFoldExpr only when Diag.

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

Tue, Mar 14, 8:03 AM · Restricted Project, Restricted Project
erichkeane accepted D145408: Fix false positive with unreachable C++ catch handlers.
Tue, Mar 14, 7:41 AM · Restricted Project, Restricted Project
erichkeane accepted D143479: [Clang] Emit error when caller cannot meet target feature requirement from always-inlining callee.

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

Tue, Mar 14, 7:25 AM · Restricted Project, Restricted Project
erichkeane added a comment to D145892: [SemaCXX]use CorrectDelayedTyposInExpr in ActOnCXXFoldExpr only when Diag.

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?

Tue, Mar 14, 7:07 AM · Restricted Project, Restricted Project
erichkeane added inline comments to D145851: [Clang][Sema] Fix incorrect deletion of default constructors for some unions.
Tue, Mar 14, 7:01 AM · Restricted Project, Restricted Project

Mon, Mar 13

erichkeane added inline comments to D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type..
Mon, Mar 13, 11:23 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
erichkeane updated subscribers of D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type..
Mon, Mar 13, 11:22 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
erichkeane added a comment to D128440: [WebAssembly] Initial support for reference type funcref in clang.

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

Mon, Mar 13, 10:17 AM · Restricted Project, Restricted Project, Restricted Project
erichkeane added a comment to D145851: [Clang][Sema] Fix incorrect deletion of default constructors for some unions.

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.

Mon, Mar 13, 10:08 AM · Restricted Project, Restricted Project
erichkeane added a comment to D145892: [SemaCXX]use CorrectDelayedTyposInExpr in ActOnCXXFoldExpr only when Diag.

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.
Mon, Mar 13, 9:20 AM · Restricted Project, Restricted Project
erichkeane added a comment to D145892: [SemaCXX]use CorrectDelayedTyposInExpr in ActOnCXXFoldExpr only when Diag.

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

Mon, Mar 13, 8:45 AM · Restricted Project, Restricted Project
erichkeane added a comment to D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type..

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?

Mon, Mar 13, 8:39 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
erichkeane added a comment to D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type..

Patch seems to be missing all the context.

Mon, Mar 13, 8:32 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
erichkeane accepted D142692: [clang] Store the template param list of an explicit variable template specialization.

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.

Mon, Mar 13, 7:28 AM · Restricted Project, Restricted Project, Restricted Project
erichkeane added a comment to D145851: [Clang][Sema] Fix incorrect deletion of default constructors for some unions.

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!)?

Mon, Mar 13, 7:10 AM · Restricted Project, Restricted Project
erichkeane accepted D145491: [clang] Replace Member Expressions During Instantiation If Necessary.

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

Mon, Mar 13, 7:07 AM · Restricted Project, Restricted Project, Restricted Project
erichkeane updated subscribers of D145284: WIP [clang] adds capabilities for SARIF to be written to file.

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

Mon, Mar 13, 6:31 AM · Restricted Project, Restricted Project, Restricted Project

Fri, Mar 10

erichkeane accepted D144403: [clang] Extract attribute plugin instantiation to function (NFC).
Fri, Mar 10, 8:39 AM · Restricted Project, Restricted Project
erichkeane accepted D144405: [clang][pp] Handle attributes defined by plugin in __has_attribute.

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.

Fri, Mar 10, 7:54 AM · Restricted Project, Restricted Project
erichkeane added a comment to D144402: [clang] Move ParsedAttrInfo from Sema to Basic (NFC).

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.

Fri, Mar 10, 6:28 AM · Restricted Project, Restricted Project
erichkeane added a comment to D144405: [clang][pp] Handle attributes defined by plugin in __has_attribute.

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

Fri, Mar 10, 6:19 AM · Restricted Project, Restricted Project
erichkeane accepted D145769: [clang] Extract ParsedAttrInfo::hasSpelling method (NFC).

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

Fri, Mar 10, 6:13 AM · Restricted Project, Restricted Project

Thu, Mar 9

erichkeane added a reverting change for rGb8a1b698afb2: [clang] fix missing initialization of original number of expansions: rGacecf68c8b7c: Revert two patches to fix GH58452 regression.
Thu, Mar 9, 9:17 AM · Restricted Project, Restricted Project
erichkeane committed rGacecf68c8b7c: Revert two patches to fix GH58452 regression (authored by erichkeane).
Revert two patches to fix GH58452 regression
Thu, Mar 9, 9:17 AM · Restricted Project, Restricted Project
erichkeane added a reverting change for rG3a0309c53674: [clang] Improve diagnostics for expansion length mismatch: rGacecf68c8b7c: Revert two patches to fix GH58452 regression.
Thu, Mar 9, 9:17 AM · Restricted Project, Restricted Project
erichkeane added a reverting change for D128095: [clang] Improve diagnostics for expansion length mismatch: rGacecf68c8b7c: Revert two patches to fix GH58452 regression.
Thu, Mar 9, 9:17 AM · Restricted Project, Restricted Project
erichkeane added a reverting change for D131802: [clang] fix missing initialization of original number of expansions: rGacecf68c8b7c: Revert two patches to fix GH58452 regression.
Thu, Mar 9, 9:17 AM · Restricted Project, Restricted Project
erichkeane closed D145605: Revert two patches to fix GH58452 regression.
Thu, Mar 9, 9:17 AM · Restricted Project, Restricted Project
erichkeane updated the diff for D145605: Revert two patches to fix GH58452 regression.

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

Thu, Mar 9, 6:45 AM · Restricted Project, Restricted Project

Wed, Mar 8

erichkeane updated the diff for D145605: Revert two patches to fix GH58452 regression.

Add tests from regression bug report.

Wed, Mar 8, 1:01 PM · Restricted Project, Restricted Project
erichkeane retitled D145605: Revert two patches to fix GH58452 regression from Revert two patches to fix PR58452 regression to Revert two patches to fix GH58452 regression.
Wed, Mar 8, 12:59 PM · Restricted Project, Restricted Project
erichkeane added a comment to D145605: Revert two patches to fix GH58452 regression.

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

Wed, Mar 8, 12:58 PM · Restricted Project, Restricted Project
erichkeane requested review of D145605: Revert two patches to fix GH58452 regression.
Wed, Mar 8, 12:57 PM · Restricted Project, Restricted Project

Tue, Mar 7

erichkeane added inline comments to D145270: Add codegen for llvm exp/exp2 elementwise builtins.
Tue, Mar 7, 9:21 AM · Restricted Project, Restricted Project
erichkeane added a comment to D145270: Add codegen for llvm exp/exp2 elementwise builtins.

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.

Tue, Mar 7, 9:13 AM · Restricted Project, Restricted Project
erichkeane added a comment to D145491: [clang] Replace Member Expressions During Instantiation If Necessary.

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.

Tue, Mar 7, 6:26 AM · Restricted Project, Restricted Project, Restricted Project

Mon, Mar 6

erichkeane added inline comments to D145408: Fix false positive with unreachable C++ catch handlers.
Mon, Mar 6, 11:25 AM · Restricted Project, Restricted Project
erichkeane committed rG9306ef9750b7: [clang][alias|ifunc]: Add a diagnostic for mangled names (authored by 0xdc03).
[clang][alias|ifunc]: Add a diagnostic for mangled names
Mon, Mar 6, 8:59 AM · Restricted Project, Restricted Project
erichkeane closed D143803: [clang][alias|ifunc]: Add a diagnostic for mangled names.
Mon, Mar 6, 8:59 AM · Restricted Project, Restricted Project
erichkeane accepted D143803: [clang][alias|ifunc]: Add a diagnostic for mangled names.

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

Mon, Mar 6, 8:09 AM · Restricted Project, Restricted Project
erichkeane added inline comments to D144405: [clang][pp] Handle attributes defined by plugin in __has_attribute.
Mon, Mar 6, 6:35 AM · Restricted Project, Restricted Project

Tue, Feb 28

erichkeane added a comment to D144405: [clang][pp] Handle attributes defined by plugin in __has_attribute.

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?

Tue, Feb 28, 2:40 PM · Restricted Project, Restricted Project
erichkeane accepted D144404: [clang] Extract function for generated part of clang::hasAttribute (NFC).
Tue, Feb 28, 2:39 PM · Restricted Project, Restricted Project