Page MenuHomePhabricator

aaron.ballman (Aaron Ballman)
User

Projects

User does not belong to any projects.

User Details

User Since
Mar 14 2013, 3:16 PM (322 w, 4 d)

Recent Activity

Today

aaron.ballman accepted D61643: [PragmaHandler][NFC] Expose `#pragma` location.

LGTM!

Tue, May 21, 7:55 AM · Restricted Project
aaron.ballman accepted D62192: [clang-tidy]: Add cert-oop54-cpp alias for bugprone-unhandled-self-assignment.

LGTM aside from some nits, thank you for this!

Tue, May 21, 7:49 AM · Restricted Project
aaron.ballman committed rG86abee8185c1: Add support for dumping AST comment nodes to JSON. (authored by aaron.ballman).
Add support for dumping AST comment nodes to JSON.
Tue, May 21, 7:37 AM
aaron.ballman added a comment to D61508: [clang-tidy] bugprone-header-guard : a simple version of llvm-header-guard.

Latest change addresses most of issues.
Outstanding is adding a test when garbage GuardStyle value is fed into config.
The trial testcase used only CHECK-*-NOT which caused complaining from test harness looking for the CHECK- cases.
Looking for something similar in the other checker tests did not turn up anything.
In general it seems like clang-tidy assumes developers get the string names of the options and their values correct and does not provide a lot of (any?) checking.

Tue, May 21, 5:12 AM · Restricted Project, Restricted Project

Yesterday

aaron.ballman added a comment to D62056: Use ASTDumper to dump the AST from clang-query.

If you wanted to expand test coverage for the changes covered in this, that would be a good thing. However, I'm not certain how testable the newly colorized output is, which is why this LG as-is.

Mon, May 20, 1:44 PM · Restricted Project
aaron.ballman added inline comments to D62065: Move dump method implementations to their respective class files.
Mon, May 20, 1:39 PM · Restricted Project
aaron.ballman accepted D62056: Use ASTDumper to dump the AST from clang-query.

LGTM

Mon, May 20, 1:39 PM · Restricted Project
aaron.ballman committed rG4aee1b5b0b9a: Add more tests for AST JSON output; NFC. (authored by aaron.ballman).
Add more tests for AST JSON output; NFC.
Mon, May 20, 1:00 PM
aaron.ballman committed rG4d05a974b7f1: Dump macro expansion information as needed when outputting the AST to JSON. (authored by aaron.ballman).
Dump macro expansion information as needed when outputting the AST to JSON.
Mon, May 20, 9:46 AM
aaron.ballman accepted D61851: [clang-tidy] New option for misc-throw-by-value-catch-by-reference.

LGTM (with the typo fix).

Mon, May 20, 7:05 AM · Restricted Project, Restricted Project, Restricted Project

Fri, May 17

aaron.ballman committed rGbbfd8d188570: Add more tests for AST JSON output; NFC. (authored by aaron.ballman).
Add more tests for AST JSON output; NFC.
Fri, May 17, 12:12 PM
aaron.ballman added inline comments to D61508: [clang-tidy] bugprone-header-guard : a simple version of llvm-header-guard.
Fri, May 17, 7:02 AM · Restricted Project, Restricted Project
aaron.ballman added inline comments to D61834: Add a Visit overload for DynTypedNode to ASTNodeTraverser.
Fri, May 17, 7:02 AM · Restricted Project, Restricted Project
aaron.ballman added inline comments to D59628: Add support for __attribute__((objc_class_stub)).
Fri, May 17, 6:45 AM · Restricted Project
aaron.ballman accepted D61834: Add a Visit overload for DynTypedNode to ASTNodeTraverser.

What will be making use of/testing this new functionality?

Any code which has a DynTypedNode and wishes to traverse it.

I envisage this as a more-flexible DynTypedNode::dump that the user does not have to implement themselves in order to use the ASTNodeTraverser.

Do we currently have any such code that's using this functionality, though? I'm mostly concerned that this is dead code with no testing, currently. The functionality itself seems reasonable enough and the code looks correct enough, so if this is part of a series of planned changes, that'd be good information to have for the review.

Ah, yes. This is supposed to be 'useful public API' like the other Visit methods for use inside and outside the codebase. A follow-up patch will use it, but it's provided for external use too anyway.

I'll add a unit test.

Fri, May 17, 6:38 AM · Restricted Project, Restricted Project
aaron.ballman added inline comments to D61837: Make it possible control matcher traversal kind with ASTContext.
Fri, May 17, 6:23 AM · Restricted Project
aaron.ballman accepted D61835: Extract ASTDumper to a header file.
  1. Anyone who wants traversal in the same way that dumping is done and who needs to call API on the instance which is provided by ASTNodeTraverser (which ASTDumper inherits) needs to use ASTDumper. For example my UI. See my EuroLLVM talk for more: https://steveire.wordpress.com/2019/04/30/the-future-of-ast-matching-refactoring-tools-eurollvm-and-accu/

Do they? Why is the ASTNodeTraverser insufficient?

It doesn't have the same behavior as ASTDumper, because ASTDumper "overrides" some Visit metthods.

I'm aware that they're different, but I may not have been sufficiently clear. I'm saying that the only public APIs I think a user should be calling are inherited from ASTNodeTraverser and not ASTDumper, so it is not required to expose ASTDumper directly, only a way to get an ASTNodeTraverser reference/pointer to an ASTDumper instance so that you get the correct virtual dispatch. e.g., ASTNodeTraverser *getSomethingThatActsLikeAnASTDumper() { return new ASTDumper; }

Perhaps. One way to implement the 'less noise' version of AST output (ie removing pointer addresses etc http://ce.steveire.com/z/HaCLeO ) is to make it an API on the TextNodeDumper. Then ASTDumper would need a forwarding API for it.

Anyway, your argument also applies to JSONNodeDumper, but you put that in a header file.

Fri, May 17, 5:57 AM · Restricted Project

Thu, May 16

aaron.ballman added inline comments to D61837: Make it possible control matcher traversal kind with ASTContext.
Thu, May 16, 12:56 PM · Restricted Project
aaron.ballman added a comment to D61835: Extract ASTDumper to a header file.
  1. Anyone who wants traversal in the same way that dumping is done and who needs to call API on the instance which is provided by ASTNodeTraverser (which ASTDumper inherits) needs to use ASTDumper. For example my UI. See my EuroLLVM talk for more: https://steveire.wordpress.com/2019/04/30/the-future-of-ast-matching-refactoring-tools-eurollvm-and-accu/

Do they? Why is the ASTNodeTraverser insufficient?

It doesn't have the same behavior as ASTDumper, because ASTDumper "overrides" some Visit metthods.

Thu, May 16, 12:36 PM · Restricted Project
aaron.ballman added inline comments to D61837: Make it possible control matcher traversal kind with ASTContext.
Thu, May 16, 12:00 PM · Restricted Project
aaron.ballman added a comment to D61835: Extract ASTDumper to a header file.

The users of the follow-up patch https://reviews.llvm.org/D61837#change-x5mxz9Lpijjs need that 'correctness', but also need the public API from ASTNodeTraverser on the instance. (That patch also extends the public API for users).

I don't see anything in the follow-up patch that actually uses the ASTDumper class though, so I'm still a bit confused.

Maybe my wording was confusing. I'll try again:

  1. Anyone who wants traversal in the same way that dumping is done must use an instance of the ASTDumper class. Is that much clear? Does my previous response clarify why that is the case?
Thu, May 16, 11:54 AM · Restricted Project
aaron.ballman added a comment to D61835: Extract ASTDumper to a header file.

I'm not certain where you're planning to go with this change (or is this the only change you're trying to make in this area?), so it's a bit hard to evaluate this patch. Can you explain a bit more about what you're ultimately trying to accomplish?

It might help if I had a better idea of which APIs you thought were ones that would help users (because my only real concern with this change is that the public interface for this class is rather unpleasant).

The reason the ASTDumper class still exists (for the purpose of dumping an AST to stream at least) is that it dumps the {Function,Var,Class}TemplateDecl 'correctly'.

Thu, May 16, 11:34 AM · Restricted Project
aaron.ballman added a comment to D61834: Add a Visit overload for DynTypedNode to ASTNodeTraverser.

What will be making use of/testing this new functionality?

Any code which has a DynTypedNode and wishes to traverse it.

I envisage this as a more-flexible DynTypedNode::dump that the user does not have to implement themselves in order to use the ASTNodeTraverser.

Thu, May 16, 11:27 AM · Restricted Project, Restricted Project
aaron.ballman accepted D61836: Move TraversalKind enum to ast_type_traits.

LGTM aside from some commenting requests.

Thu, May 16, 7:40 AM · Restricted Project
aaron.ballman added a comment to D61837: Make it possible control matcher traversal kind with ASTContext.

Thanks for this -- it looks like really interesting functionality! I've mostly found nits thus far, but did have a question about clang-query support for it.

Thu, May 16, 7:36 AM · Restricted Project
aaron.ballman added a comment to D61834: Add a Visit overload for DynTypedNode to ASTNodeTraverser.

What will be making use of/testing this new functionality?

Thu, May 16, 6:01 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D61835: Extract ASTDumper to a header file.

I'm not certain where you're planning to go with this change (or is this the only change you're trying to make in this area?), so it's a bit hard to evaluate this patch. Can you explain a bit more about what you're ultimately trying to accomplish?

Thu, May 16, 5:56 AM · Restricted Project

Tue, May 14

aaron.ballman accepted D61874: [clang-tidy] Fix invalid fixit for readability-static-accessed-through-instance (bug 40544).

LGTM!

Tue, May 14, 5:48 AM · Restricted Project, Restricted Project, Restricted Project
aaron.ballman committed rGd06f3917913d: Add a new language mode for C2x; enable [[attribute]] support by default in C2x. (authored by aaron.ballman).
Add a new language mode for C2x; enable [[attribute]] support by default in C2x.
Tue, May 14, 5:08 AM
aaron.ballman closed D61370: Add a C2x mode and allow attributes in it.

I've commit in r360667, thanks!

Tue, May 14, 5:07 AM

Mon, May 13

aaron.ballman committed rG3fdd2b1bd2da: Removing an unused member variable; NFC. (authored by aaron.ballman).
Removing an unused member variable; NFC.
Mon, May 13, 3:29 PM
aaron.ballman closed D60910: [WIP] Dumping the AST to JSON.

Commit in r360622.

Mon, May 13, 2:44 PM
aaron.ballman committed rG2ce598a44a35: Introduce the ability to dump the AST to JSON. (authored by aaron.ballman).
Introduce the ability to dump the AST to JSON.
Mon, May 13, 2:38 PM
aaron.ballman added a comment to D60910: [WIP] Dumping the AST to JSON.

If you're happy with these two conditions, then I have no concerns with this moving forward:

  • There is no implied stability for the content or format of the dump between major releases, other than that it be valid JSON; this should be stated explicitly in the documentation. (Compatibility between patch releases seems like something we can work out with the release manager, but I'm inclined to say we should make a best-effort attempt to preserve it.) If people want to build tools on this rather than on one of our stable APIs, they should expect to be broken in some way on every major release.
  • There is no requirement for people maintaining the AST (changing or adding AST nodes) to update the dump output for modified AST nodes to show any new information -- unlike the existing -ast-dump, this is not just for debugging, but we should be able to treat it as if it were. Perhaps a better way to put it: there is no requirement that the information in this dump is complete, but the information that is dumped should be correct.

    If you want stronger guarantees than that, then we should have a broader discussion to establish some community buy-in.
Mon, May 13, 1:33 PM
aaron.ballman updated the diff for D60910: [WIP] Dumping the AST to JSON.

Switched to using the JSON streaming interface rather than a custom solution
Updated the test cases for the new formatting

Mon, May 13, 1:27 PM
aaron.ballman added a comment to D61370: Add a C2x mode and allow attributes in it.

Ping x2.

Mon, May 13, 1:04 PM
aaron.ballman accepted D61619: Make language option `GNUAsm` discoverable with `__has_extension` macro..

LGTM!

Mon, May 13, 12:50 PM · Restricted Project
aaron.ballman added a comment to D24892: [clang-tidy] Add option "LiteralInitializers" to cppcoreguidelines-pro-type-member-init.

Do I wait for @alexfh to turn his red into a green, too?

Mon, May 13, 12:45 PM · Restricted Project
aaron.ballman added inline comments to D61851: [clang-tidy] New option for misc-throw-by-value-catch-by-reference.
Mon, May 13, 12:42 PM · Restricted Project, Restricted Project, Restricted Project
aaron.ballman added reviewers for D60543: [clang] Update isDerivedFrom to support Objective-C classes 🔍: klimek, alexfh.

Adding some more AST matcher reviewers to see if there are other options that we've not considered.

Mon, May 13, 9:04 AM · Restricted Project
aaron.ballman added a comment to D60543: [clang] Update isDerivedFrom to support Objective-C classes 🔍.

I did some digging and I believe there are two approaches that we can take to extend isDerivedFrom to support Objective-C classes.

Option 1: Match on Common Ancestor Declaration Type:
Convert isDerivedFrom to match on the common ancestor declaration type, NamedDecl, and return false for
declaration types other than CXXRecordDecl and ObjCInterfaceDecl.

Advantages:
• Largely works in-place without requiring major changes to matchers built on top of isDerivedFrom.

Disadvantages:
• Allows nonsensical matchers, e.g., functionDecl(isDerivedFrom("X")).

Option 2: Convert to Polymorphic Matcher:
Convert isDerivedFrom to a polymorphic matcher supporting CXXRecordDecl and ObjCInterfaceDecl.

Advantages:
• Restricts usage of isDerivedFrom to CXXRecordDecl and ObjCInterfaceDecl.

Disadvantages:
• Potentially requires many or all matchers using isDerivedFrom to refactor to adapt to the polymorphic matcher interface.

Evaluation
I have been going back and forth as to which approach is superior. Option 1 promotes source stability at the cost of usability while
option 2 seems to promote usability at the cost of source stability. I exported a portrayal of option 1 for consideration as it
required less invasive changes. I can see arguments supporting both approaches.

What do you think? Which of the two approaches do you think we should go with? Is there another approach that I have not thought of?

Mon, May 13, 9:02 AM · Restricted Project

Fri, May 10

aaron.ballman committed rGcc55804be050: Removing an unused member variable; NFC. (authored by aaron.ballman).
Removing an unused member variable; NFC.
Fri, May 10, 11:27 AM
aaron.ballman accepted D61700: [clang-tidy] readability-redundant-declaration: fix false positive with C "extern inline".

LGTM with some testing nits.

Fri, May 10, 9:30 AM · Restricted Project, Restricted Project
aaron.ballman closed D56160: [clang-tidy] modernize-use-trailing-return-type check.
  • fixed formatting
  • fixed function names in tests
  • added -fexceptions to test arguments
  • fixed typo in release notes
Fri, May 10, 9:26 AM · Restricted Project
aaron.ballman committed rG61c0daa0076e: Recommit r360345 with fixes (was reverted in r360348). (authored by aaron.ballman).
Recommit r360345 with fixes (was reverted in r360348).
Fri, May 10, 9:24 AM

Thu, May 9

aaron.ballman reopened D56160: [clang-tidy] modernize-use-trailing-return-type check.

@aaron.ballman I do not have commit privileges and I would be very thankful, if you could commit this patch for me! Thank you!

I've commit for you in r360345, thank you for the patch!

Thu, May 9, 8:06 AM · Restricted Project
aaron.ballman committed rG0268083329c3: Revert r360345 and r360346, as they are not passing the testbots. (authored by aaron.ballman).
Revert r360345 and r360346, as they are not passing the testbots.
Thu, May 9, 8:05 AM
aaron.ballman committed rGf58a5c8803e3: Fixing a link in the release notes to appease the Sphinx bot. (authored by aaron.ballman).
Fixing a link in the release notes to appease the Sphinx bot.
Thu, May 9, 7:59 AM
aaron.ballman committed rG8e015b2e94f8: Add the modernize-use-trailing-return check to rewrite function signatures to… (authored by aaron.ballman).
Add the modernize-use-trailing-return check to rewrite function signatures to…
Thu, May 9, 7:49 AM
aaron.ballman closed D56160: [clang-tidy] modernize-use-trailing-return-type check.
Thu, May 9, 7:48 AM · Restricted Project
aaron.ballman added a comment to D56160: [clang-tidy] modernize-use-trailing-return-type check.

@aaron.ballman I do not have commit privileges and I would be very thankful, if you could commit this patch for me! Thank you!

Thu, May 9, 7:48 AM · Restricted Project
aaron.ballman accepted D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment.

LGTM!

Thu, May 9, 5:46 AM · Restricted Project, Restricted Project, Restricted Project
aaron.ballman added inline comments to D56160: [clang-tidy] modernize-use-trailing-return-type check.
Thu, May 9, 5:46 AM · Restricted Project
aaron.ballman added a comment to D56160: [clang-tidy] modernize-use-trailing-return-type check.

@aaron.ballman and @JonasToth: Thank you for the patience and all the feedback! It means a great deal to me to have a patch accepted here!

Thu, May 9, 5:34 AM · Restricted Project
aaron.ballman accepted D24892: [clang-tidy] Add option "LiteralInitializers" to cppcoreguidelines-pro-type-member-init.

LGTM

Thu, May 9, 5:32 AM · Restricted Project
aaron.ballman accepted D61566: Fix for bug 41747: AST Printer doesn't print nested name specifier for out of scope record definitions.

LGTM!

Thu, May 9, 5:06 AM · Restricted Project

Wed, May 8

aaron.ballman committed rGef317e0561ae: Allow test to pass after 2030. (authored by aaron.ballman).
Allow test to pass after 2030.
Wed, May 8, 6:42 AM
aaron.ballman closed D52967: Extend shelf-life by 70 years.

Can someone please merge this change?

Wed, May 8, 6:41 AM · Restricted Project
aaron.ballman committed rG6de5576af7f4: Allow 'static' storage specifier on an out-of-line class member template… (authored by aaron.ballman).
Allow 'static' storage specifier on an out-of-line class member template…
Wed, May 8, 6:26 AM
aaron.ballman closed D59280: Allow 'static' storage specifier on an out-of-line class member template declaration in MSVCCompat mode.

Thank you for updating the test! One minor nit with the way the test case is written, but otherwise LGTM.

Thanks Aaron. I don't think I have commit right. Could you please submit the patches?

Wed, May 8, 6:26 AM
aaron.ballman accepted D60629: [clang-tidy] Change the namespace for llvm checkers from 'llvm' to 'llvm_check'.

This LGTM, but you may want to wait a day or so to see if @alexfh has any remaining concerns.

Wed, May 8, 5:49 AM · Restricted Project, Restricted Project
aaron.ballman accepted D56160: [clang-tidy] modernize-use-trailing-return-type check.

Aside from a formatting issue, this LGTM, thank you!

Wed, May 8, 5:47 AM · Restricted Project

Tue, May 7

aaron.ballman accepted D59280: Allow 'static' storage specifier on an out-of-line class member template declaration in MSVCCompat mode.

Thank you for updating the test! One minor nit with the way the test case is written, but otherwise LGTM.

Tue, May 7, 12:51 PM
aaron.ballman accepted D61642: [clang-tidy] Do not show incorrect fix in modernize-make-unique.

LGTM!

Tue, May 7, 11:31 AM · Restricted Project, Restricted Project
aaron.ballman accepted D61350: [clang-tidy] New check calling out uses of +new in Objective-C code.

LGTM modulo the comments from @gribozavr.

Tue, May 7, 11:29 AM · Restricted Project, Restricted Project
aaron.ballman accepted D55793: [clang-tidy] Add duplicated access specifier readability check (PR25403).

LGTM!

Tue, May 7, 9:39 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D61370: Add a C2x mode and allow attributes in it.

Ping.

Tue, May 7, 9:11 AM
aaron.ballman added inline comments to D61566: Fix for bug 41747: AST Printer doesn't print nested name specifier for out of scope record definitions.
Tue, May 7, 9:05 AM · Restricted Project
aaron.ballman added inline comments to D61642: [clang-tidy] Do not show incorrect fix in modernize-make-unique.
Tue, May 7, 9:04 AM · Restricted Project, Restricted Project
aaron.ballman added inline comments to D59280: Allow 'static' storage specifier on an out-of-line class member template declaration in MSVCCompat mode.
Tue, May 7, 8:59 AM
aaron.ballman added inline comments to D60910: [WIP] Dumping the AST to JSON.
Tue, May 7, 8:25 AM
aaron.ballman updated the diff for D60910: [WIP] Dumping the AST to JSON.

Updating based on review feedback.

Tue, May 7, 8:24 AM
aaron.ballman committed rGc635eb725e6e: Add an explicit triple to this test to hopefully appease the build bots. (authored by aaron.ballman).
Add an explicit triple to this test to hopefully appease the build bots.
Tue, May 7, 7:39 AM
aaron.ballman committed rGbb6e7b365436: Allow field offset lookups in types with incomplete arrays within libclang. (authored by aaron.ballman).
Allow field offset lookups in types with incomplete arrays within libclang.
Tue, May 7, 6:59 AM
aaron.ballman closed D61239: [libclang] Allow field offset lookups in types with incomplete arrays..

I've commit in r360147, thank you for the patch!

Tue, May 7, 6:58 AM · Restricted Project
aaron.ballman added a comment to D60956: [Sema] Fix the lookup for a declaration conflicting with an enumerator.

I found a few nits, but generally think this LG. However, I think @rsmith should sign off on it just in case I've misinterpreted something along the way.

Tue, May 7, 6:56 AM · Restricted Project
aaron.ballman added inline comments to D61350: [clang-tidy] New check calling out uses of +new in Objective-C code.
Tue, May 7, 6:43 AM · Restricted Project, Restricted Project
aaron.ballman added inline comments to D33841: [clang-tidy] redundant keyword check.
Tue, May 7, 6:38 AM · Restricted Project

Mon, May 6

aaron.ballman added inline comments to D56160: [clang-tidy] modernize-use-trailing-return-type check.
Mon, May 6, 1:23 PM · Restricted Project
aaron.ballman added a comment to D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment.

Ping.
Is it good to go or is there anything else I need to include in this patch?

Mon, May 6, 12:12 PM · Restricted Project, Restricted Project, Restricted Project
aaron.ballman added inline comments to D33841: [clang-tidy] redundant keyword check.
Mon, May 6, 11:36 AM · Restricted Project
aaron.ballman accepted D61260: [clang-tidy] Extend bugprone-sizeof-expression to check sizeof(pointers to structures).

LGTM!

Mon, May 6, 11:22 AM · Restricted Project, Restricted Project, Restricted Project
aaron.ballman added a comment to D61508: [clang-tidy] bugprone-header-guard : a simple version of llvm-header-guard.

Hi trixirt and thanks for the patch!

I would rather like to generalize the llvm check to allow different styles and then alias the general version with different configurations. Introducing this code duplication does not sound like a good idea to me.
The documentation fixes you make can be done in a separate patch to keep things clean.

Mon, May 6, 11:07 AM · Restricted Project, Restricted Project

Sun, May 5

aaron.ballman requested changes to D61552: [clang] Adapt ASTMatcher to explicit(bool) specifier.

You should also regenerate the documentation by running clang/docs/tools/dump_ast_matchers.py.

Sun, May 5, 6:00 AM · Restricted Project

Fri, May 3

aaron.ballman added inline comments to D59280: Allow 'static' storage specifier on an out-of-line class member template declaration in MSVCCompat mode.
Fri, May 3, 10:31 PM
aaron.ballman added inline comments to D55125: [clang-tidy] Fix a false positive in misc-redundant-expression check.
Fri, May 3, 10:46 AM · Restricted Project
aaron.ballman added inline comments to D59280: Allow 'static' storage specifier on an out-of-line class member template declaration in MSVCCompat mode.
Fri, May 3, 7:26 AM
aaron.ballman accepted D61422: [clang-tidy] Extend bugprone-sizeof-expression check to detect sizeof misuse in pointer arithmetic.

Out of curiosity, have you run this over any large code bases to see what the false positive and true positive rate is?

Neither false, nor true positives found so far. I ran it on several open-source projects.

Fri, May 3, 5:53 AM · Restricted Project, Restricted Project
aaron.ballman accepted D61480: Added an AST matcher for declarations that are in the `std` namespace.

LGTM!

Fri, May 3, 5:26 AM · Restricted Project
aaron.ballman requested changes to D61480: Added an AST matcher for declarations that are in the `std` namespace.

You should also update Registry.cpp to add this to clang-query and you should also regenerate the documentation by running clang\docs\tools\dump_ast_matchers.py.

Fri, May 3, 1:15 AM · Restricted Project
aaron.ballman added a comment to D59280: Allow 'static' storage specifier on an out-of-line class member template declaration in MSVCCompat mode.

I would still like to see the second RUN line added to the test so we can ensure we don't regress the behavior.

Fri, May 3, 12:27 AM

Thu, May 2

aaron.ballman added a comment to D61422: [clang-tidy] Extend bugprone-sizeof-expression check to detect sizeof misuse in pointer arithmetic.

Out of curiosity, have you run this over any large code bases to see what the false positive and true positive rate is?

Thu, May 2, 9:31 AM · Restricted Project, Restricted Project
aaron.ballman accepted D61444: Do not warn on switches over enums that do not use [[maybe_unused]] enumerators.

LGTM!

Thu, May 2, 9:27 AM · Restricted Project
aaron.ballman added a comment to D61239: [libclang] Allow field offset lookups in types with incomplete arrays..

LGTM, thank you! That's strange that clang-format would remove the newline from the end of the file. I don't get that behavior when I try it, so it makes me wonder what's different.

Yeah, my mistake. It was there after all. The output from clang-format looked the same as what I had before, but I think I just didn't catch the newline when copy-pasting the diff the first time :)

Thu, May 2, 1:34 AM · Restricted Project

Wed, May 1

aaron.ballman accepted D61097: [Sema] Emit warning for visibility attribute on internal-linkage declaration.

LGTM, thank you!

Wed, May 1, 12:53 PM · Restricted Project, Restricted Project
aaron.ballman accepted D61239: [libclang] Allow field offset lookups in types with incomplete arrays..

LGTM, thank you! That's strange that clang-format would remove the newline from the end of the file. I don't get that behavior when I try it, so it makes me wonder what's different.

Wed, May 1, 12:53 PM · Restricted Project
aaron.ballman added inline comments to D61239: [libclang] Allow field offset lookups in types with incomplete arrays..
Wed, May 1, 7:54 AM · Restricted Project
aaron.ballman added a comment to D61288: [Diagnostics] Implemented support for -Wswitch-default.

I am not familiar with clang-tidy’s codebase and I personally prefer good compiler warnings than dependency on another tool (clang-tidy). I mean the cases when diagnostic check is easy to do in the compiler.

And in semaexpr we have all we need and it is simple to do it.

But if majority of reviewers disagree with compiler warning, I will close this revision.

Wed, May 1, 7:24 AM · Restricted Project
aaron.ballman added a comment to D61288: [Diagnostics] Implemented support for -Wswitch-default.

Some coding guidelines may require switch to have always default label. Even if devs know that default is not reachable, they can add default: abort(); or assert to increase safety (and warning will be silenced).

Yes, it not suitable to be enabled by default, but I still think it is good to have it.

Wed, May 1, 6:34 AM · Restricted Project
aaron.ballman added a comment to D61239: [libclang] Allow field offset lookups in types with incomplete arrays..
  • holding of on adding helper method until hearing back.
Wed, May 1, 6:14 AM · Restricted Project