[PATCH] D57267: [AST] Factor out the logic of the various Expr::Ignore*

aaron.ballman added a comment. In D57267#1391101 , @riccibruno wrote: > > I don't think there was an explicit reason beyond "I didn't need to do it > > at the time". So probably just an oversight on my part. I don't know the > > code nearly as well as @r

[PATCH] D57984: PR40642: Fix determination of whether the final statement of a statementexpression is a discarded-value expression.

aaron.ballman added a comment. Thank you for working on this! It looks like we did have to add the new AST node after all, but the changes seem pretty good. Should anything be updated in the AST dumper for this (such as printing whether a Stmt is a value-producing statement or not)?

[PATCH] D45978: dllexport const variables must have external linkage.

aaron.ballman added a comment. It looks like the patch got mucked up somehow, I only see three testing files in the patch now? Comment at: test/Sema/dllexport.c:168 + +// CHECK: @y = common dso_local dllexport global i32 0, align 4 + Nothing runs FileCheck in

[PATCH] D57948: [Sema] Fix a regression introduced in "[AST][Sema] Remove CallExpr::setNumArgs"

aaron.ballman added inline comments. Comment at: lib/AST/Expr.cpp:1272-1274 + for (unsigned I = NumArgs; I != NumArgsAllocated; ++I) { +TrailingArgs[I] = nullptr; } Could use `std::fill()` here to remove the for-loop. Your call whether that looks cleaner

[PATCH] D57918: Add an attribute that causes clang to emit fortified calls to C stdlib functions

aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:987-989 +The first argument to the attribute is the type passed to +`__builtin_object_size`, and the second is the flag that the fortified format +functions accept. Maybe I'm b

[PATCH] D33841: [clang-tidy] redundant keyword check

aaron.ballman added inline comments. Comment at: docs/clang-tidy/checks/readability-redundant-extern.rst:4 +readability-redundant-extern += + The underlining here is too long. Comment at: docs/clang-tidy/checks/reada

[PATCH] D45978: dllexport const variables must have external linkage.

aaron.ballman added inline comments. Comment at: test/Sema/dllexport-1.c:1-4 +// RUN: %clang_cc1 -triple i686-win32 -emit-llvm -fms-extensions -std=c99 < %s| FileCheck %s +// RUN: %clang_cc1 -triple x86_64-win32 -emit-llvm -fms-extensions -std=c11 < %s | FileCheck %s +// RUN: %

[PATCH] D17444: PR26672: [MSVC] Clang does not recognize "static_assert" keyword in C mode

aaron.ballman added a comment. > Also, I suppose this is really actually an -fms-compatibility thing not > -fms-extensions because it takes an identifier from outside the implementer's > namespace. I just verified, it really does take the identifier and it does not treat `static_assert` as a m

[PATCH] D57918: Add an attribute that causes clang to emit fortified calls to C stdlib functions

aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM aside from some very minor nits. Comment at: clang/include/clang/Basic/AttrDocs.td:987-989 +The first argument to the attribute is the type passed to +`__builtin_object_size`, and the second is the flag t

[PATCH] D57984: PR40642: Fix determination of whether the final statement of a statementexpression is a discarded-value expression.

aaron.ballman added a comment. Considering that this has been fertile ground for buggy edge cases, should we revert my original changes from the 8.0 branch to give this more time to bake on trunk before releasing? Or do you think this is fine to move onto the release branch once finalized? =

[PATCH] D58095: [clang-tidy] Make google-objc-function-naming ignore implicit functions 🙈

aaron.ballman added inline comments. Herald added a subscriber: jdoerfert. Comment at: clang-tools-extra/test/clang-tidy/google-objc-function-naming.m:10 +// function would be declared in a system header. +int printf(const char *, ...); // NOLINT(google-objc-function-naming) + -

[PATCH] D45978: dllexport const variables must have external linkage.

aaron.ballman added inline comments. Comment at: test/Sema/dllexport-1.c:1 +// RUN: %clang_cc1 -triple i686-win32 -emit-llvm -fms-extensions -std=c99 < %s| FileCheck %s +// RUN: %clang_cc1 -triple x86_64-win32 -emit-llvm -fms-extensions -std=c11 < %s | FileCheck %s

[PATCH] D45978: dllexport const variables must have external linkage.

aaron.ballman added a comment. Oops, sorry, I think I added some comments to a previous revision of the review. To recap: 1. Remove the CHECK line from test/Sema/dllexport-1.cpp 2. Remove the second RUN line from test/Sema/dllexport-1.cpp and test/Sema/dllexport-2.cpp 3. Possibly remove the tri

[PATCH] D57948: [Sema] Fix a regression introduced in "[AST][Sema] Remove CallExpr::setNumArgs"

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57948/new/ https://reviews.llvm.org/D57948 ___

[PATCH] D57080: [clang-tidy] Delete obsolete objc-property-declaration options ✂️

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57080/new/ https://reviews.llvm.org/D57080

[PATCH] D58095: [clang-tidy] Make google-objc-function-naming ignore implicit functions 🙈

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! Comment at: clang-tools-extra/test/clang-tidy/Inputs/Headers/stdio.h:1 +#ifndef _STDIO_H_ +#define _STDIO_H_ stephanemoore wrote: > steph

[PATCH] D45978: dllexport const variables must have external linkage.

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D45978/new/ https://reviews.llvm.org/D45978 ___ cfe-commits mailing lis

[PATCH] D57855: [analyzer] Reimplement checker options

aaron.ballman added inline comments. Herald added a subscriber: jdoerfert. Comment at: include/clang/StaticAnalyzer/Checkers/CheckerBase.td:13-14 +/// Describes a checker or package option type. This is important for validating +/// user supplied inputs. +class CmdLineOptionTy

[PATCH] D58152: [Sema] Delay checking whether objc_designated_initializer is being applied to an init method

aaron.ballman added inline comments. Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5252 const ParsedAttr &AL) { + auto *Ctx = D->getDeclContext(); + Please don't use `auto` here. Comment at: clang/lib

[PATCH] D57984: PR40642: Fix determination of whether the final statement of a statementexpression is a discarded-value expression.

aaron.ballman added inline comments. Comment at: include/clang/Parse/Parser.h:374 +/// This context is at the top level of a GNU statement expression. +InStmtExpr = 0x4, + It's a bit strange that the previous two enumerators are "Allow" and this is "In".

[PATCH] D45978: dllexport const variables must have external linkage.

aaron.ballman added inline comments. Comment at: lib/Sema/SemaDecl.cpp:11370 +// In Microsoft C++ mode, a const variable defined in namespace scope has +// external linkage by default if the variable is declared with thakis wrote: > Even in unnamed name

[PATCH] D58091: Customize warnings for missing built-in type

aaron.ballman added a reviewer: rsmith. aaron.ballman added a comment. If I'm following along properly, it sounds like we want to disable this warning largely because it can appear in header files attempting to declare the functions in question -- but I wonder why those diagnostics are happening

[PATCH] D58091: Customize warnings for missing built-in type

aaron.ballman added a comment. In D58091#1396397 , @jdoerfert wrote: > In D58091#1396382 , @aaron.ballman > wrote: > > > - but I wonder why those diagnostics are happening in the first place. It > > seems like the

[PATCH] D58152: [Sema] Delay checking whether objc_designated_initializer is being applied to an init method

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM with a testing request. Comment at: clang/test/SemaObjC/attr-designated-init.m:433-438 +- (instancetype)foo +__attribute__((objc_designated_initializer

[PATCH] D57984: PR40642: Fix determination of whether the final statement of a statementexpression is a discarded-value expression.

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! Comment at: lib/Parse/ParseStmt.cpp:1090-1091 +R = handleExprStmt(Res, SubStmtCtx); +if (R.isUsable()) + R = Actions.ProcessStmtA

[PATCH] D17444: [MSVC] Recognize "static_assert" keyword in C mode

aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D17444/new/ https://reviews.llvm.org/D17444 ___ cfe-commits mailing list cfe-comm

[PATCH] D57948: [Sema] Fix a regression introduced in "[AST][Sema] Remove CallExpr::setNumArgs"

aaron.ballman added a subscriber: hansw. aaron.ballman added a comment. In D57948#1399403 , @riccibruno wrote: > (Following up on a discussion on IRC) I have looked at what would be needed > to revert the set of patches which introduced this change, but t

[PATCH] D58292: Add support for importing ChooseExpr AST nodes.

aaron.ballman added a reviewer: aaron.ballman. aaron.ballman added a comment. Please be sure to update Registry.cpp to expose the new AST matcher to the dynamic matcher infrastructure, regenerate the AST matcher documentation (run clang\docs\tools\dump_ast_matchers.py), and add tests for the mat

[PATCH] D45978: dllexport const variables must have external linkage.

aaron.ballman added inline comments. Comment at: lib/Sema/SemaDecl.cpp:11370 +// In Microsoft C++ mode, a const variable defined in namespace scope has +// external linkage by default if the variable is declared with zahiraam wrote: > aaron.ballman wrot

[PATCH] D58365: [attributes] Add a MIG server routine attribute.

aaron.ballman added a comment. > @aaron.ballman, are you actually interested in reviewing these attributes? > 'Cause George added you on his reviews but i totally understand that these > are fairly exotic. Yeah, I'd like to review these attributes -- thanks for double-checking with me! =

[PATCH] D58095: [clang-tidy] Make google-objc-function-naming ignore implicit functions 🙈

aaron.ballman added a comment. Still LGTM, thanks for adding the license snippet! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58095/new/ https://reviews.llvm.org/D58095 ___ cfe-commits mailing list c

[PATCH] D58292: Add support for importing ChooseExpr AST nodes.

aaron.ballman added inline comments. Comment at: lib/AST/ASTImporter.cpp:6140 +ExpectedStmt ASTNodeImporter::VisitChooseExpr(ChooseExpr *E) { + auto Imp = importSeq(E->getCond(), E->getLHS(), E->getRHS(), + E->getBuiltinLoc(), E->getRParenLoc(), E->getType(

[PATCH] D58365: [attributes] Add a MIG server routine attribute.

aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8702 +def warn_mig_server_routine_does_not_return_kern_return_t : Warning< + "'%0' attribute only applies to functions that return a kernel return code">, + InGroup; --

[PATCH] D58365: [attributes] Add a MIG server routine attribute.

aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM with a minor documentation nit. Comment at: clang/include/clang/Basic/AttrDocs.td:4049 +it will be automatically applied to overrides if the method is virtual. The +attribute can be written using C++11 sy

[PATCH] D58292: Add support for importing ChooseExpr AST nodes.

aaron.ballman added inline comments. Comment at: lib/AST/ASTImporter.cpp:6140 +ExpectedStmt ASTNodeImporter::VisitChooseExpr(ChooseExpr *E) { + auto Imp = importSeq(E->getCond(), E->getLHS(), E->getRHS(), + E->getBuiltinLoc(), E->getRParenLoc(), E->getType(

[PATCH] D58292: Add support for importing ChooseExpr AST nodes.

aaron.ballman added inline comments. Comment at: lib/AST/ASTImporter.cpp:6140 +ExpectedStmt ASTNodeImporter::VisitChooseExpr(ChooseExpr *E) { + auto Imp = importSeq(E->getCond(), E->getLHS(), E->getRHS(), + E->getBuiltinLoc(), E->getRParenLoc(), E->getType(

[PATCH] D58292: Add support for importing ChooseExpr AST nodes.

aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58292/new/ https://reviews.llvm.org/D58292 ___ cfe-commits mailing list cfe-commits@lists.llvm.

[PATCH] D58292: Add support for importing ChooseExpr AST nodes.

aaron.ballman added inline comments. Comment at: lib/AST/ASTImporter.cpp:6160 + // condition-dependent. + bool CondIsTrue = false; + if (!E->isConditionDependent()) tmroeder wrote: > aaron.ballman wrote: > > a_sidorin wrote: > > > aaron.ballman wrote: > > > >

[PATCH] D58292: Add support for importing ChooseExpr AST nodes.

aaron.ballman added inline comments. Comment at: lib/AST/ASTImporter.cpp:6160 + // condition-dependent. + bool CondIsTrue = false; + if (!E->isConditionDependent()) tmroeder wrote: > a_sidorin wrote: > > aaron.ballman wrote: > > > tmroeder wrote: > > > > aaron

[PATCH] D58292: Add support for importing ChooseExpr AST nodes.

aaron.ballman added inline comments. Comment at: lib/AST/ASTImporter.cpp:6140 +ExpectedStmt ASTNodeImporter::VisitChooseExpr(ChooseExpr *E) { + auto Imp = importSeq(E->getCond(), E->getLHS(), E->getRHS(), + E->getBuiltinLoc(), E->getRParenLoc(), E->getType(

[PATCH] D36354: [clang-tidy] Implement type-based check for `gsl::owner`

aaron.ballman added inline comments. Comment at: clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp:202 + + // Initializer of class constructors, that initialize owners. + if (OwnerInitializer) { Can remove the comma. Comment at: clang-tidy/c

[PATCH] D33852: Enable __declspec(selectany) on linux

aaron.ballman added a comment. In https://reviews.llvm.org/D33852#853410, @Prazek wrote: > Sorry for so late fixes, but it would be good to put it in 5.0 I do not think this should be in 5.0, as I believe we're only accepting regression fixes at this point. Comment at: incl

[PATCH] D33852: Enable __declspec(selectany) on linux

aaron.ballman added a comment. In https://reviews.llvm.org/D33852#854176, @Prazek wrote: > In https://reviews.llvm.org/D33852#853982, @aaron.ballman wrote: > > > In https://reviews.llvm.org/D33852#853410, @Prazek wrote: > > > > > Sorry for so late fixes, but it would be good to put it in 5.0 > >

[PATCH] D36272: [CodeGen][x86_64] Enable 'force_align_arg_pointer' attribute at x86_64

aaron.ballman added a comment. In https://reviews.llvm.org/D36272#851709, @erichkeane wrote: > Aaron would likely know better than me... but could it be the spelling type > should be GCC instead of GNU? Yes, this should be spelled with `GCC` rather than `GNU`, as it's a documented GCC attribu

[PATCH] D33852: Enable __declspec(selectany) on linux

aaron.ballman added inline comments. Comment at: include/clang/Basic/Attr.td:2477 let Spellings = [Declspec<"selectany">, GCC<"selectany">]; let Documentation = [Undocumented]; } aaron.ballman wrote: > Since we're drastically modifying what platforms this

[PATCH] D36892: [clang-tidy] check_clang_tidy.py: support CHECK-NOTES prefix

aaron.ballman added a comment. Instead of CHECK-NOTES, do we want to extend CHECK-MESSAGES to handle `note` in addition to `warning` and `error`? I'd prefer to keep the number of "magic" names as low as possible so I have to remember less stuff when writing or reviewing tests. Repository: r

[PATCH] D36586: [clang-tidy] hicpp bitwise operations on signed integers

aaron.ballman added inline comments. Comment at: clang-tidy/hicpp/SignedBitwiseCheck.cpp:23 + const auto SignedIntegerOperand = + expr(ignoringImpCasts(hasType(isSignedInteger(.bind("signed_operand"); + JonasToth wrote: > JonasToth wrote: > > aaron.bal

[PATCH] D37060: [clang-tidy] Improve hicpp-exception-baseclass to handle generic code better

aaron.ballman added inline comments. Comment at: clang-tidy/hicpp/ExceptionBaseclassCheck.cpp:41 + "'std::exception'") + << BadThrow->getSubExpr()->getType() << BadThrow->getSourceRange(); Can you add a test tha

[PATCH] D36892: [clang-tidy] check_clang_tidy.py: support CHECK-NOTES prefix

aaron.ballman added a comment. > I do agree that it makes sense to keep it as low as possible, but also i see > a clear logic between all thee current checks: Thank you for the explanation. I think that makes sense to me, but I'd like to hear from @alexfh before accepting. C

[PATCH] D36586: [clang-tidy] hicpp bitwise operations on signed integers

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. Thanks! LGTM! https://reviews.llvm.org/D36586 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin

[PATCH] D37060: [clang-tidy] Improve hicpp-exception-baseclass to handle generic code better

aaron.ballman added inline comments. Comment at: clang-tidy/hicpp/ExceptionBaseclassCheck.cpp:41 + "'std::exception'") + << BadThrow->getSubExpr()->getType() << BadThrow->getSourceRange(); JonasToth wrote: > aar

[PATCH] D37060: [clang-tidy] Improve hicpp-exception-baseclass to handle generic code better

aaron.ballman added inline comments. Comment at: test/clang-tidy/hicpp-exception-baseclass.cpp:9 +class deep_hierarchy : public derived_exception {}; class non_derived_exception {}; JonasToth wrote: > aaron.ballman wrote: > > JonasToth wrote: > > > aaron.ballm

[PATCH] D37060: [clang-tidy] Improve hicpp-exception-baseclass to handle generic code better

aaron.ballman added subscribers: sbenza, klimek. aaron.ballman added inline comments. Comment at: test/clang-tidy/hicpp-exception-baseclass.cpp:9 +class deep_hierarchy : public derived_exception {}; class non_derived_exception {}; JonasToth wrote: > aaron.ball

[PATCH] D36272: [CodeGen][x86_64] Enable 'force_align_arg_pointer' attribute at x86_64

aaron.ballman added a comment. In https://reviews.llvm.org/D36272#856722, @erichkeane wrote: > In https://reviews.llvm.org/D36272#856040, @anatol.pomozov wrote: > > > Hi Eric, thank you for your reply. Both these triples are currently broken, > > with my change and without. > > > > The attribute

[PATCH] D37060: [clang-tidy] Improve hicpp-exception-baseclass to handle generic code better

aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM! https://reviews.llvm.org/D37060 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33852: Enable __declspec(selectany) on linux

aaron.ballman added inline comments. Comment at: include/clang/Basic/Attr.td:2477 let Spellings = [Declspec<"selectany">, GCC<"selectany">]; let Documentation = [Undocumented]; } Prazek wrote: > aaron.ballman wrote: > > aaron.ballman wrote: > > > Since we'

[PATCH] D36272: [CodeGen][x86_64] Enable 'force_align_arg_pointer' attribute at x86_64

aaron.ballman accepted this revision. aaron.ballman added a comment. In https://reviews.llvm.org/D36272#856857, @erichkeane wrote: > In https://reviews.llvm.org/D36272#856849, @anatol.pomozov wrote: > > > > if we corrected the attribute spelling to be GCC instead of GNU and added > > > some docu

[PATCH] D37308: Interface class with uuid base record

aaron.ballman added inline comments. Comment at: lib/Sema/SemaDeclCXX.cpp:2403-2406 +static bool IsBasePublicInterface(const CXXRecordDecl *RD, + AccessSpecifier spec) { + return RD->isInterface() && spec == AS_public; +} I'm not

[PATCH] D36272: [CodeGen][x86_64] Enable 'force_align_arg_pointer' attribute at x86_64

aaron.ballman added a comment. In https://reviews.llvm.org/D36272#857005, @anatol.pomozov wrote: > Hi Aaron. Thank you very much for reviewing this patch. > > I'll be glad to add documentation for 'force_align_arg_pointer' attribute and > upload another patch. But before doing it I would like to

[PATCH] D37346: Register linkageSpecDecl matcher

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! https://reviews.llvm.org/D37346 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman

[PATCH] D37312: Add documentation for force_align_arg_pointer function attribute

aaron.ballman added inline comments. Comment at: include/clang/Basic/AttrDocs.td:2812 +Legacy x86 code uses 4-byte stack alignment. Newer aligned SSE instructions +(like 'movaps') that work with stack require operands to be 16-byte aligned. +This attribute realigns stack in the f

[PATCH] D37308: Interface class with uuid base record

aaron.ballman added a comment. In https://reviews.llvm.org/D37308#857607, @zahiraam wrote: > Removed the helper function. > > If RD (base class) has uuid attribute, we want to ensure that the interface > doesn't have attributes. Otherwise cases like: > > class __declspec(uuid("--

[PATCH] D33852: Enable __declspec(selectany) on linux

aaron.ballman added inline comments. Comment at: include/clang/Basic/AttrDocs.td:3154 + +def SelectAnyDocs : Documentation { + let Content = [{This attribute makes global symbol have a weak definition I think you need to set the `Category` as well. To test thi

[PATCH] D37308: Interface class with uuid base record

aaron.ballman added a comment. In https://reviews.llvm.org/D37308#858035, @zahiraam wrote: > The test case you gave doesn't compile. It returns the diagnostic. CL has the > same behavior. I don't think it is because of the dllimport. My test case was based off your example code, not something

[PATCH] D37312: Add documentation for force_align_arg_pointer function attribute

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! Thank you for working on this! https://reviews.llvm.org/D37312 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http:/

[PATCH] D33852: Enable __declspec(selectany) on linux

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. Assuming no sphinx issues with the docs, this LGTM, thank you! Comment at: include/clang/Basic/AttrDocs.td:3154 + +def SelectAnyDocs : Documentation { + let C

[PATCH] D37436: Initial implementation of C attributes (WG14 N2137)

aaron.ballman created this revision. Herald added a subscriber: javed.absar. WG14 has demonstrated sincere interest in adding C++-style attributes to C for C2x, and have added the proposal to their SD-3 document tracking features which are possibly desired for the next version of the standard. T

[PATCH] D33825: [clang-tidy] signal handler must be plain old function check

aaron.ballman added a comment. Aside from a few small nits, this looks reasonable. Have you run it over any large code bases that use signals to test the quality of the check? Comment at: docs/clang-tidy/checks/cert-msc54-cpp.rst:23 +extern "C" void cpp_signal_handler(int

[PATCH] D33826: [clang-tidy] avoid pointer cast to more strict alignment check

aaron.ballman added a comment. Have you run this check over any large code bases to see what the quality of the diagnostics are? Comment at: docs/clang-tidy/checks/cert-exp36-c.rst:9 + +This check is the same as `-Wcast-align`, except it checks for `reinterpret_cast` as well

[PATCH] D37620: [Sema] Put tautological comparison of unsigned and zero into it's own flag

aaron.ballman added a comment. > Does this need test? Yes, it does -- I'd add one with two RUN lines, one with the flag and one without it to make sure you only get the diagnostics in one case. Repository: rL LLVM https://reviews.llvm.org/D37620 __

[PATCH] D37620: [Sema] Put tautological comparison of unsigned and zero into it's own flag

aaron.ballman added a comment. In https://reviews.llvm.org/D37620#864684, @sammccall wrote: > Thanks! > Unless I'm missing something, the current patch just adds the category, but > doesn't actually recategorize the warning, right? The diagnostics in DiagnosticSemaKinds.td are changed to be i

[PATCH] D37620: [Sema] Put tautological comparison of unsigned and zero into it's own flag

aaron.ballman requested changes to this revision. aaron.ballman added a comment. This revision now requires changes to proceed. The bulk of the patch LGTM as well, but it should still have a test case. Repository: rL LLVM https://reviews.llvm.org/D37620

[PATCH] D33852: Enable __declspec(selectany) on linux

aaron.ballman added inline comments. Comment at: include/clang/Basic/AttrDocs.td:3154 + +def SelectAnyDocs : Documentation { + let Content = [{This attribute makes global symbol have a weak definition Prazek wrote: > aaron.ballman wrote: > > Prazek wrote: > > >

[PATCH] D37620: [Sema] Put tautological comparison of unsigned and zero into it's own flag

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. Please run the test through clang-format, but otherwise LGTM! Repository: rL LLVM https://reviews.llvm.org/D37620 ___ cfe-commit

[PATCH] D36354: [clang-tidy] Implement type-based check for `gsl::owner`

aaron.ballman added inline comments. Comment at: clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp:140 + + assert(CheckExecuted == true && + "Non of the subroutines did execute, logic error in matcher!"); Can assert `CheckExecuted` and drop the explicit

[PATCH] D37308: Interface class with uuid base record

aaron.ballman added inline comments. Comment at: lib/Sema/SemaDeclCXX.cpp:2385 + return RD->isStruct() && RD->hasAttr() && + RD->getName() == "IUnknown" && + (RD->getAttr())->getGuid() == This should probably also ensure that the class is in the

[PATCH] D36672: [clang-tidy] readability-non-const-parameter: fixit on all function declarations

aaron.ballman added inline comments. Comment at: clang-tidy/readability/NonConstParameterCheck.cpp:147 +if (const auto *Parent = Par->getParentFunctionOrMethod()) { + if (const auto *F = dyn_cast(Parent)) { +const auto ParDecl = AndersRonnholm wr

[PATCH] D37572: [clang-tidy] SuspiciousEnumUsageCheck bugfix

aaron.ballman added inline comments. Comment at: clang-tidy/misc/SuspiciousEnumUsageCheck.cpp:46 + return llvm::APSInt::compareValues(E1->getInitVal(), + E2->getInitVal()) == -1; }); I would test for `

[PATCH] D37624: add support for -fno-instrument-functions and -finstrument-functions-exclude-{file, function}-list= to match gcc options.

aaron.ballman added inline comments. Comment at: lib/CodeGen/CodeGenFunction.cpp:425 /// instrumented with __cyg_profile_func_* calls -bool CodeGenFunction::ShouldInstrumentFunction() { +bool CodeGenFunction::ShouldInstrumentFunction(llvm::Function *Fn) { + typedef std::vector:

[PATCH] D37572: [clang-tidy] SuspiciousEnumUsageCheck bugfix

aaron.ballman added inline comments. Comment at: test/clang-tidy/misc-suspicious-enum-usage.cpp:122 +struct a> { + enum { ah = ad::m, + ai = ae::m, alexfh wrote: > aaron.ballman wrote: > > This seems like a lot of complicated code for the test case -- ca

[PATCH] D33852: Enable __declspec(selectany) on linux

aaron.ballman added inline comments. Comment at: include/clang/Basic/AttrDocs.td:3154 + +def SelectAnyDocs : Documentation { + let Content = [{This attribute makes global symbol have a weak definition majnemer wrote: > aaron.ballman wrote: > > Prazek wrote: > >

[PATCH] D33852: Enable __declspec(selectany) on linux

aaron.ballman added inline comments. Comment at: include/clang/Basic/AttrDocs.td:3154 + +def SelectAnyDocs : Documentation { + let Content = [{This attribute makes global symbol have a weak definition aaron.ballman wrote: > majnemer wrote: > > aaron.ballman wro

[PATCH] D37572: [clang-tidy] SuspiciousEnumUsageCheck bugfix

aaron.ballman added inline comments. Comment at: test/clang-tidy/misc-suspicious-enum-usage.cpp:122 +struct a> { + enum { ah = ad::m, + ai = ae::m, alexfh wrote: > alexfh wrote: > > aaron.ballman wrote: > > > alexfh wrote: > > > > aaron.ballman wrote: >

[PATCH] D36354: [clang-tidy] Implement type-based check for `gsl::owner`

aaron.ballman added inline comments. Comment at: test/clang-tidy/cppcoreguidelines-owning-memory.cpp:39 + return new int(42); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: returning a 'gsl::owner<>' from a function but not declaring it; return type is 'int *' +} ---

[PATCH] D37643: Add objcImplementationDecl matcher

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM, thanks! https://reviews.llvm.org/D37643 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin

[PATCH] D37436: Initial implementation of C attributes (WG14 N2137)

aaron.ballman updated this revision to Diff 114447. aaron.ballman marked 2 inline comments as done. aaron.ballman added a comment. Updated based on Richard's comments and some further discussion on IRC. https://reviews.llvm.org/D37436 Files: include/clang/Basic/Attr.td include/clang/Basic/A

[PATCH] D37436: Initial implementation of C attributes (WG14 N2137)

aaron.ballman added a comment. In https://reviews.llvm.org/D37436#867287, @rsmith wrote: > If this is just supposed to be an experiment to get feedback on the feature, > then I don't think we should be treating it as a different attribute syntax > at all. Rather, I think we > just want to per

[PATCH] D37436: Initial implementation of C attributes (WG14 N2137)

aaron.ballman updated this revision to Diff 114827. aaron.ballman marked 3 inline comments as done. aaron.ballman added a comment. Updates based on review feedback. https://reviews.llvm.org/D37436 Files: include/clang/Basic/Attr.td include/clang/Basic/Attributes.h include/clang/Basic/Lang

[PATCH] D36354: [clang-tidy] Implement type-based check for `gsl::owner`

aaron.ballman added inline comments. Comment at: clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp:141 + assert(CheckExecuted && + "Non of the subroutines executed, logic error in matcher!"); +} Non -> None Comment at: clang-tidy/cppc

[PATCH] D37629: [Sema] Move some stuff into -Wtautological-unsigned-enum-zero-compare

aaron.ballman added inline comments. Comment at: lib/Sema/SemaChecking.cpp:8602 - if (op == BO_LT && isNonBooleanUnsignedValue(LHS) && IsZero(S, RHS)) { -S.Diag(E->getOperatorLoc(), diag::warn_lunsigned_always_true_comparison) - << "< 0" << "false" << HasEnumType(LHS)

[PATCH] D33826: [clang-tidy] avoid pointer cast to more strict alignment check

aaron.ballman added a comment. In https://reviews.llvm.org/D33826#866170, @lebedev.ri wrote: > In https://reviews.llvm.org/D33826#866161, @JonasToth wrote: > > > There is an exception to the general rule (EXP36-C-EX2), stating that the > > result of `malloc` and friends is allowed to be casted t

[PATCH] D33826: [clang-tidy] avoid pointer cast to more strict alignment check

aaron.ballman added a comment. In https://reviews.llvm.org/D33826#867155, @rjmccall wrote: > In https://reviews.llvm.org/D33826#866170, @lebedev.ri wrote: > > > In https://reviews.llvm.org/D33826#866161, @JonasToth wrote: > > > > > There is an exception to the general rule (EXP36-C-EX2), stating

[PATCH] D37308: Interface class with uuid base record

aaron.ballman added inline comments. Comment at: lib/Sema/SemaDeclCXX.cpp:2376 +/// \brief Tests if the __interface base is public. +static bool IsDeclPublicInterface(const CXXRecordDecl *RD, The comment does not match the behavior of the function. ==

[PATCH] D36354: [clang-tidy] Implement type-based check for `gsl::owner`

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM, with a few last typos. Comment at: test/clang-tidy/cppcoreguidelines-owning-memory.cpp:354 + +// FIXME: Same typededcution problems +template ---

[PATCH] D36354: [clang-tidy] Implement type-based check for `gsl::owner`

aaron.ballman accepted this revision. aaron.ballman added a comment. Good catch on the deleted constructor -- LGTM still! https://reviews.llvm.org/D36354 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/l

[PATCH] D33826: [clang-tidy] avoid pointer cast to more strict alignment check

aaron.ballman added a comment. In https://reviews.llvm.org/D33826#868167, @lebedev.ri wrote: > In https://reviews.llvm.org/D33826#868085, @aaron.ballman wrote: > > > In https://reviews.llvm.org/D33826#866170, @lebedev.ri wrote: > > > > > In https://reviews.llvm.org/D33826#866161, @JonasToth wrote

[PATCH] D37436: Initial implementation of C attributes (WG14 N2137)

aaron.ballman added a comment. In https://reviews.llvm.org/D37436#868295, @hfinkel wrote: > In https://reviews.llvm.org/D37436#867965, @aaron.ballman wrote: > > > In https://reviews.llvm.org/D37436#867287, @rsmith wrote: > > > > > If this is just supposed to be an experiment to get feedback on th

[PATCH] D37436: Initial implementation of C attributes (WG14 N2137)

aaron.ballman added a comment. In https://reviews.llvm.org/D37436#869350, @hfinkel wrote: > In https://reviews.llvm.org/D37436#868333, @aaron.ballman wrote: > > > In https://reviews.llvm.org/D37436#868295, @hfinkel wrote: > > > > > In https://reviews.llvm.org/D37436#867965, @aaron.ballman wrote:

[PATCH] D37436: Initial implementation of C attributes (WG14 N2137)

aaron.ballman updated this revision to Diff 115015. aaron.ballman added a comment. Added full context, no other changes from previous patch. https://reviews.llvm.org/D37436 Files: clang/include/clang/Basic/Attr.td clang/include/clang/Basic/Attributes.h clang/include/clang/Basic/LangOption

[PATCH] D37436: Initial implementation of C attributes (WG14 N2137)

aaron.ballman added a comment. In https://reviews.llvm.org/D37436#869462, @hfinkel wrote: > I think that I misunderstood your concern. Let me see if I can summarize your > position: You believe that, when GCC implements this syntax in C, they will > audit their attributes and not support all of

<    8   9   10   11   12   13   14   15   16   17   >