[PATCH] D52386: [Lexer] Add udefined_behavior_sanitizer feature

rsmith added a comment. In https://reviews.llvm.org/D52386#1254483, @leonardchan wrote: > In https://reviews.llvm.org/D52386#1254437, @rsmith wrote: > > > I'm not at all convinced that this is a good thing. There isn't such a > > thing as "undefined behavior sanitizer". Rather, there are a whole

[PATCH] D49511: [Sema/Attribute] Check for noderef attribute

rsmith added inline comments. Comment at: include/clang/Basic/AttrDocs.td:3513 + int x = 2; + int &y = x; // warning: 'noderef' can only be used on an array or pointer type + Should `noderef` appear somewhere in this example? :) Comment at:

[PATCH] D52386: [Lexer] Add udefined_behavior_sanitizer feature

rsmith added a comment. I'm not at all convinced that this is a good thing. There isn't such a thing as "undefined behavior sanitizer". Rather, there are a whole bunch of different checks that fall under the same umbrella. This test seems on the surface to be about as meaningless as `__has_feat

[PATCH] D52849: [SEMA] split ExtWarn dupl-decl-spec's into Extension and ExtWarn

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. LGTM, thanks! Repository: rC Clang https://reviews.llvm.org/D52849 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.o

[PATCH] D52839: Inform AST's UnaryOperator of FENV_ACCESS

rsmith added a comment. Maybe take this very slightly further so it becomes testable: change the expression evaluator (lib/AST/ExprConstant.cpp) so it treats floating-point operations as non-constant if they're potentially-overflowing. You also need to update lib/Serialization/AST{Reader,Writer

[PATCH] D52695: [clang][Parse] Diagnose useless null statements (PR39111)

rsmith added inline comments. Comment at: include/clang/Basic/DiagnosticGroups.td:164 +def ExtraSemiStmt : DiagGroup<"extra-semi-stmt">; +def EmptyInitStatement : DiagGroup<"empty-init-stmt">; def ExtraSemi : DiagGroup<"extra-semi", [CXX98CompatExtraSemi, I thin

[PATCH] D52137: Added warning for unary minus used with unsigned type

rsmith added inline comments. Comment at: test/Sema/unary-minus-integer-impcast-2.c:1 +// RUN: %clang_cc1 %s -verify -Wconversion -fsyntax-only -triple i386-pc-linux-gnu + You can combine the two tests together into a single file using a #ifdef to detect which

[PATCH] D52248: [SEMA] ignore duplicate declaration specifiers from typeof exprs

rsmith added a comment. Downgrading the C89 warning for duplicate qualifiers from typedefs / __typeof from `ExtWarn` to `Extension` seems very reasonable to me. However, I don't see a justification for removing the warning for duplicate explicit (non-typedef) qualifiers in C99, nor for downgrad

[PATCH] D52695: [clang][Parse] Diagnose useless null statements (PR39111)

rsmith added a comment. Like Eli, I don't see much value in this warning. But it doesn't seem much different from `-Wextra-semi` in that regard for languages in which such extra semicolons are generally valid -- I expect both warnings will generally diagnose typos and little else. Does this wa

[PATCH] D52529: [Frontend] Delete -print-decl-contexts

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Looks good to me. Please first mail cfe-dev announcing this change and wait a day or so for anyone using this feature to speak up before committing. Comment at: test/Coverag

[PATCH] D52750: [Diagnostics] Check for integer overflow in array size expressions

rsmith added a comment. This is still evaluating the expression twice. To evaluate it only once, you'll need to sink the checks into `Sema::VerifyIntegerConstantExpression`'s call to `EvaluateKnownConstInt`. (That should also remove the redundant diagnostics noise in the language modes where si

[PATCH] D52137: Added warning for unary minus used with unsigned type

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Thanks! Comment at: lib/Sema/SemaChecking.cpp:10899 + if (TargetRange.Width > SourceRange.Width) +if (auto *UO = dyn_cast(E)) Add braces to these oute

[PATCH] D52750: [Diagnostics] Check for integer overflow in array size expressions

rsmith added a comment. We're going to try evaluating the array size anyway (in `isArraySizeVLA`). It would be much better to produce the overflow warnings as part of that evaluation rather than adding an extra evaluation step to produce them. https://reviews.llvm.org/D52750 ___

[PATCH] D52529: [Frontend] Simplify -print-decl-contexts with DeclNodes.inc

rsmith added a comment. Do we need `-print-decl-contexts` at all? It's not exposed by the driver, not tested, not documented, has a very strange output format (the bracketing character supplies some information about the kind of declaration, but it's not clear *what* information), and it doesn'

[PATCH] D52339: Support enums with a fixed underlying type in all language modes

rsmith added a comment. In https://reviews.llvm.org/D52339#1249457, @aaron.ballman wrote: > In https://reviews.llvm.org/D52339#1249432, @erik.pilkington wrote: > > > Ping! This doesn't really affect CUDA or OpenCL more than any change to the > > compiler, so I don't think it makes sense to block

[PATCH] D52576: llvm::sort(C.begin(), C.end(), ...) -> llvm::sort(C, ...)

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Thanks for the cleanup! Maybe consider running `clang-format-diff` on this? Comment at: lib/Sema/SemaOverload.cpp:10813-10814 - llvm::sort(Cands.begin(), Cands.end(), + l

[PATCH] D52248: [SEMA] ignore duplicate declaration specifiers from typeof exprs

rsmith added a comment. The right way to produce diagnostics under pedantic mode is to model them as Extension or ExtWarn in the .td file, not by checking the Pedantic diagnostic option directly. If this is an intentional GNU extension too, that makes things a bit more complex (one approach wou

[PATCH] D52446: Remove Found.clear() to silent bugprone-use-after-move after rC342925

rsmith added a comment. I think the check is wrong here. We need this `clear` call to reinitialize the object after moving from it. Maybe that function should be annotated as reinitializing the object instead? Repository: rC Clang https://reviews.llvm.org/D52446 _

[PATCH] D51789: [clang] Add the exclude_from_explicit_instantiation attribute

rsmith added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:4683-4686 + "Member '%0' marked with 'exclude_from_explicit_instantiation' attribute is " + "not defined but an explicit template instantiation declaration exists. " + "Reliance on this

[PATCH] D52321: [CUDA] Fixed parsing of optional template-argument-list.

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Parse/ParseTemplate.cpp:949-950 GreaterThanIsOperatorScope G(GreaterThanIsOperator, false); -if (Tok.isNot(tok::greater) && Tok.isNot(tok::

[PATCH] D50948: lambdas in modules: change storage for LambdaDefinitionData

rsmith added a comment. We don't want to allocate storage for the lambda fields for non-lambda classes, which is why we use distinct base classes. Is the problem you're trying to solve here that we fake a definition in AST deserialization before we know whether the class is a lambda? If so, tha

[PATCH] D51910: [Modules] Add platform feature to requires clause

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: lib/Basic/Module.cpp:81 + // 2, Environment + // 3. Platform-Environment + if (Platform == Feature || Target.getTriple().getOSName() == Feature ||

[PATCH] D52137: Added warning for unary minus used with unsigned type

rsmith added a comment. In https://reviews.llvm.org/D52137#1236065, @Quuxplusone wrote: > In https://reviews.llvm.org/D52137#1236053, @rsmith wrote: > > > I think we can and should do better about false positives here. If you move > > this check to SemaChecking, you can produce the warning in a

[PATCH] D52137: Added warning for unary minus used with unsigned type

rsmith added a comment. I think we can and should do better about false positives here. If you move this check to SemaChecking, you can produce the warning in a context where you know what the final type is -- I don't think there's any reason to warn if the final type is signed and no wider tha

[PATCH] D51789: [clang] Add the exclude_from_explicit_instantiation attribute

rsmith added a comment. Looks good other than the warning, which I don't yet understand. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:4683-4686 + "Member '%0' marked with 'exclude_from_explicit_instantiation' attribute is " + "not defined but an explicit templ

[PATCH] D30882: Add a callback for __has_include and use it for dependency scanning

rsmith accepted this revision. rsmith added inline comments. Comment at: clang/lib/Frontend/DependencyFile.cpp:340 +return; + StringRef Filename = File->getName(); + if (!FileMatchesDepCriteria(Filename.data(), FileType)) vsapsai wrote: > rsmith wrote: > >

[PATCH] D51910: [Modules] Add platform feature to requires clause

rsmith added inline comments. Comment at: docs/Modules.rst:476-477 + +*platform-environment* + A platform-environment variant (e.g. ``linux-gnueabi``, ``windows-msvc``) is available. What is the reason to allow these to be combined into a single feature name

[PATCH] D30882: Add a callback for __has_include and use it for dependency scanning

rsmith added inline comments. Comment at: clang/lib/Frontend/DependencyFile.cpp:340 +return; + StringRef Filename = File->getName(); + if (!FileMatchesDepCriteria(Filename.data(), FileType)) Should we really be using the (arbitrary) name of the file from th

[PATCH] D30882: Add a callback for __has_include and use it for dependency scanning

rsmith added inline comments. Comment at: lib/Frontend/DependencyFile.cpp:325 +void DFGImpl::HasInclude(SourceLocation Loc, const FileEntry *File) { + if (!File) +return; vsapsai wrote: > rsmith wrote: > > Have you thought about whether we should add a depen

[PATCH] D51808: [CUDA] Ignore uncallable functions when we check for usual deallocators.

rsmith added inline comments. Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:551 if (Decl->isMain() || !Decl->isUserProvided() || -Decl->isUsualDeallocationFunction() || -Decl->isCopyAssignmentOperator() || Decl->isMoveAssignm

[PATCH] D51789: [clang] Add the no_extern_template attribute

rsmith added a comment. In https://reviews.llvm.org/D51789#1235100, @ldionne wrote: > In https://reviews.llvm.org/D51789#1235096, @rsmith wrote: > > > OK, so the semantics of this attribute are "explicit instantiation > > declarations or definitions applied to the enclosing class do not apply to

[PATCH] D51789: [clang] Add the no_extern_template attribute

rsmith added a comment. In https://reviews.llvm.org/D51789#1234903, @ldionne wrote: > I think now's a good time to bikeshed the name of the attribute if you have > other suggestions. OK, so the semantics of this attribute are "explicit instantiation declarations or definitions applied to the

[PATCH] D51905: Front-end of the implementation of the interleaving algorithm

rsmith added inline comments. Comment at: clang/include/clang/Driver/CC1Options.td:356-357 +HelpText<"Enable VTable interleaving">; +def disable_vtable_interleaving : Flag<["-"], "disable-vtable-interleaving">, +HelpText<"Disable VTable interleaving">; //===

[PATCH] D52064: [Sema] Add a note pointing to the first use of an implicit capture

rsmith added inline comments. Comment at: clang/lib/Sema/SemaLambda.cpp:1427 if (RefResult.isInvalid()) return ExprError(); Expr *Ref = RefResult.get(); This early exit leaves your CodeSynthesisContext on the stack. Consider using RAII?

[PATCH] D51855: [constexpr] Fix ICE when memcpy() is given a pointer to an incomplete array

rsmith added a comment. Thank you! Comment at: include/clang/Basic/DiagnosticASTKinds.td:172 "non-trivially-copyable type %1">; +def note_constexpr_memcpy_incompletetype : Note< + "cannot constant evaluate '%select{memcpy|memmove}0' between objects of " Nit

[PATCH] D51333: Diagnose likely typos in include statements

This revision was automatically updated to reflect the committed changes. Closed by commit rL342177: Diagnose likely typos in #include directives. (authored by rsmith, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D51333?vs=165361&id=

[PATCH] D51333: Diagnose likely typos in include statements

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Looks good (with a couple of minor code style adjustments). Do you need someone to commit this for you? In https://reviews.llvm.org/D51333#1233042, @sammccall wrote: > One thing I'd like to d

[PATCH] D51997: [clang] Make sure attributes on member classes are applied properly

rsmith added inline comments. Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:1268 + // Instantiate the attributes on both the template declaration and the associated record declaration. + SemaRef.InstantiateAttrsForDecl(TemplateArgs, Pattern, Inst, LateAttrs, Star

[PATCH] D51333: Diagnose likely typos in include statements

rsmith added a comment. Thanks, some comments but the approach here looks great. Comment at: include/clang/Basic/DiagnosticLexKinds.td:428-429 +def err_pp_file_not_found_typo_not_fatal +: Error<"'%0' file not found due to leading or trailing non-alphanumeric " +

[PATCH] D51650: Implement target_clones multiversioning

rsmith added inline comments. Comment at: lib/CodeGen/CodeGenModule.cpp:961-970 if (const auto *FD = dyn_cast(ND)) if (FD->isMultiVersion() && !OmitMultiVersionMangling) { if (FD->isCPUDispatchMultiVersion() || FD->isCPUSpecificMultiVersion()) AppendCPUSpe

[PATCH] D51650: Implement target_clones multiversioning

rsmith added inline comments. Comment at: include/clang/Basic/Attr.td:2031-2042 +mutable unsigned ActiveArgIndex = 0; +void AdvanceActiveArgIndex() const { + ++ActiveArgIndex; + while(ActiveArgIndex < featuresStrs_size()) { +if (std::find(featuresStrs_be

[PATCH] D51817: Do not use optimized atomic libcalls for misaligned atomics.

This revision was automatically updated to reflect the committed changes. Closed by commit rL341734: Do not use optimized atomic libcalls for misaligned atomics. (authored by rsmith, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.org/D51817 F

[PATCH] D51817: Do not use optimized atomic libcalls for misaligned atomics.

This revision was automatically updated to reflect the committed changes. Closed by commit rC341734: Do not use optimized atomic libcalls for misaligned atomics. (authored by rsmith, committed by ). Changed prior to commit: https://reviews.llvm.org/D51817?vs=164517&id=164544#toc Repository:

[PATCH] D51817: Do not use optimized atomic libcalls for misaligned atomics.

rsmith added inline comments. Comment at: lib/CodeGen/CGAtomic.cpp:949 case AtomicExpr::AO__opencl_atomic_compare_exchange_strong: case AtomicExpr::AO__atomic_load_n: case AtomicExpr::AO__atomic_store_n: efriedma wrote: > Is there any particular re

[PATCH] D51812: Simplify CheckFallThroughForBody

rsmith added a comment. I'm not a fan of the duplication introduced here, but the new code is definitely more obvious. On balance, this seems like a small improvement, so let's go for it. Comment at: b/llvm/tools/clang/lib/Sema/AnalysisBasedWarnings.cpp:542-544 + // cpu_disp

[PATCH] D51817: Do not use optimized atomic libcalls for misaligned atomics.

rsmith created this revision. rsmith added reviewers: jyknight, t.p.northover. Herald added a subscriber: jfb. The optimized (__atomic_foo_) libcalls assume that the atomic object is properly aligned, so should never be called on an underaligned object. This addresses one of several problems iden

[PATCH] D51789: [WIP][clang] Add the no_extern_template attribute

rsmith added a comment. It'd be good to test that `[[no_extern_template]]` affects instantiation, not just code generation (eg, put something in the body of the entity that will trigger an error if instantiated, and check that the diagnostic is produced at the right times). Repository: rL L

[PATCH] D51608: [modules] when deserializing method, ensure it has correct exception spec

rsmith added a comment. Fixed in r341499. Repository: rC Clang https://reviews.llvm.org/D51608 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51608: [modules] when deserializing method, ensure it has correct exception spec

rsmith added a comment. I had a look at fixing this, and it... snowballed into a much larger change than I was expecting. I have a patch on the way. Repository: rC Clang https://reviews.llvm.org/D51608 ___ cfe-commits mailing list cfe-commits@li

[PATCH] D45319: [Atomics] warn about misaligned atomic accesses using libcalls

rsmith added a comment. Herald added a subscriber: jfb. There's still something wrong here: in the cases where we produce these warnings, we still produce calls to `__atomic_*_` library functions. Those functions are specified to only work on properly-aligned inputs, so that's wrong. And there

[PATCH] D51200: Introduce per-callsite inline intrinsics

rsmith added a comment. Please also teach constant expression evaluation (lib/AST/ExprConstant.cpp) to look through these builtins when evaluating. https://reviews.llvm.org/D51200 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists

[PATCH] D50947: lambdas in modules: make PendingFakeDefinitionData a set

rsmith added inline comments. Comment at: lib/Serialization/ASTReaderDecl.cpp:1766 + +// FIXME: handle serialized lambdas assert(!DD.IsLambda && !MergeDD.IsLambda && "faked up lambda definition?"); Did you mean to add this FIXME? Comme

[PATCH] D51608: [modules] when deserializing method, ensure it has correct exception spec

rsmith added a comment. I think this is addressing a symptom rather than the cause. The bug appears to be that when parsing a.h, we end up with inconsistent exception specifications along the redeclaration chain. It looks like `Sema::AdjustDestructorExceptionSpec` doesn't do the right thing whe

[PATCH] D51507: Allow all supportable attributes to be used with #pragma clang attribute.

This revision was automatically updated to reflect the committed changes. Closed by commit rL341437: Allow all supportable non-type attributes to be used with #pragma clang… (authored by rsmith, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.o

[PATCH] D51329: [Attribute/Diagnostics] Print macro instead of whole attribute for address_space

rsmith added a comment. I think this patch is nearly ready to commit. However... if you want to handle macros that specify a sequence of attributes, we should switch to treating the macro name as a separate type sugar node rather than modeling it as a name for the `AttributedType` node. Up to y

[PATCH] D51200: Introduce per-callsite inline intrinsics

rsmith added a comment. +rjmccall for CodeGen changes https://reviews.llvm.org/D51200 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51200: Introduce per-callsite inline intrinsics

rsmith added a comment. > Support for constructor calls (`CXXTemporaryExpr`) should also be possible, > but is not the part of this patch. (You should handle the base class (`CXXConstructExpr`) that describes the semantics, rather than the derived class (`CXXTemporaryObjectExpr`) that describe

[PATCH] D45898: [SemaCXX] Mark destructor as referenced

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: lib/Sema/SemaInit.cpp:1827-1829 + if (SemaRef.DiagnoseUseOfDecl(Destructor, Loc)) +return false; + return true; Usual Clang convention

[PATCH] D51333: Diagnose likely typos in include statements

rsmith added a comment. In https://reviews.llvm.org/D51333#1220878, @christylee wrote: > In https://reviews.llvm.org/D51333#1219938, @rsmith wrote: > > > Instead of guessing whether the corrected filename would be valid, why not > > strip off the leading and trailing non-alphanumeric characters,

[PATCH] D51507: Allow all supportable attributes to be used with #pragma clang attribute.

rsmith updated this revision to Diff 163584. rsmith added a comment. Don't add #pragma clang attribute support for 'mode' and 'ext_vector_type'. Repository: rC Clang https://reviews.llvm.org/D51507 Files: include/clang/Basic/Attr.td test/Misc/pragma-attribute-supported-attributes-list.te

[PATCH] D51507: Allow all supportable attributes to be used with #pragma clang attribute.

rsmith marked an inline comment as done. rsmith added a comment. Based on the comments so far, my inclination is to allow `#pragma clang attribute` on all these attributes, including the highlighted ones where it's hard to think of cases where it'd be useful. I think our policy should be that w

[PATCH] D45898: [SemaCXX] Mark destructor as referenced

rsmith added inline comments. Comment at: lib/Sema/SemaInit.cpp:830 + + for (FieldDecl *FD : CXXRD->fields()) +if (auto *CXXRDMember = FD->getType()->getAsCXXRecordDecl()) rsmith wrote: > I think this now checks too much: only those subobjects for which we f

[PATCH] D37035: Implement __builtin_LINE() et. al. to support source location capture.

rsmith added a comment. This is in good shape. Some relatively minor comments (plus there are some unrelated whitespace changes in a few files); the only nontrivial comment I have is that I think it'd be cleaner to cache `StringLiteral*`s rather than adding a new form of `LValueBase` for the ca

[PATCH] D51507: Allow all supportable attributes to be used with #pragma clang attribute.

rsmith marked an inline comment as done. rsmith added inline comments. Comment at: test/Misc/pragma-attribute-supported-attributes-list.test:27-32 +// CHECK-NEXT: CUDAConstant (SubjectMatchRule_variable) +// CHECK-NEXT: CUDADevice (SubjectMatchRule_function, SubjectMatchRule_var

[PATCH] D51507: Allow all supportable attributes to be used with #pragma clang attribute.

rsmith added a comment. +@tra for CUDA attributes Comment at: test/Misc/pragma-attribute-supported-attributes-list.test:1 // RUN: clang-tblgen -gen-clang-test-pragma-attribute-supported-attributes -I%src_include_dir %src_include_dir/clang/Basic/Attr.td -o - | FileCheck %s -

[PATCH] D30009: Add support for '#pragma clang attribute'

rsmith added a comment. In https://reviews.llvm.org/D30009#1218669, @dexonsmith wrote: > In https://reviews.llvm.org/D30009#1218647, @rsmith wrote: > > > If we really want an "only documented attribtues can use this feature" rule > > (and I'm really not convinced that is a useful rule to enforce

[PATCH] D51507: Allow all supportable attributes to be used with #pragma clang attribute.

rsmith created this revision. rsmith added reviewers: aaron.ballman, arphaman. We previously disallowed use of undocumented attributes with #pragma clang attribute, but the justification for doing so was weak and it prevented many reasonable use cases. Repository: rC Clang https://reviews.llv

[PATCH] D51333: Diagnose likely typos in include statements

rsmith added a comment. Instead of guessing whether the corrected filename would be valid, why not strip off the leading and trailing non-alphanumeric characters, look up the resulting filename, and find out? If we did that, then not only could we be a lot more confident that we'd found the fil

[PATCH] D50353: Remove deprecated API

rsmith accepted this revision. rsmith added a comment. Thank you for doing this! Repository: rC Clang https://reviews.llvm.org/D50353 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commi

[PATCH] D51473: Improve attribute documentation to list which spellings are used in which syntaxes.

rsmith marked an inline comment as done. rsmith added inline comments. Comment at: utils/TableGen/ClangAttrEmitter.cpp:3881 +SpellingKind K = (SpellingKind)Kind; +// FIXME: Why are Microsoft spellings not listed? +if (K == SpellingKind::Microsoft) aar

[PATCH] D51473: Improve attribute documentation to list which spellings are used in which syntaxes.

This revision was automatically updated to reflect the committed changes. Closed by commit rL341097: Improve attribute documentation to list which spellings are used in which… (authored by rsmith, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm

[PATCH] D51473: Improve attribute documentation to list which spellings are used in which syntaxes.

rsmith created this revision. rsmith added a reviewer: aaron.ballman. Herald added a subscriber: cfe-commits. Instead of listing all the spellings (including attribute namespaces) in the section heading, only list the actual attribute names there, and list the spellings in the supported syntaxes t

[PATCH] D30009: Add support for '#pragma clang attribute'

rsmith added a comment. In https://reviews.llvm.org/D30009#1218647, @rsmith wrote: > One consequence of this patch (see https://reviews.llvm.org/rL341002) is that > adding documentation to an existing attribute //changes the behavior of > Clang//. That does not seem acceptable to me: documentat

[PATCH] D30009: Add support for '#pragma clang attribute'

rsmith added a comment. Herald added a subscriber: llvm-commits. One consequence of this patch (see https://reviews.llvm.org/rL341002) is that adding documentation to an existing attribute //changes the behavior of Clang//. That does not seem acceptable to me: documentation improvements should

[PATCH] D51190: [AttrDocs]: document gnu_inline function attribute

rsmith added a comment. Looks good, thanks for improving our documentation! Repository: rC Clang https://reviews.llvm.org/D51190 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51190: [AttrDocs]: document gnu_inline function attribute

rsmith added inline comments. Comment at: include/clang/Basic/AttrDocs.td:3518 +the behavior of ``inline`` in both C99 inline semantics and C++ inline +semantics). + Might be useful to be a bit more explicit about how it differs: > [...] is just a hint. In parti

[PATCH] D51329: [Attribute/Diagnostics] Print macro instead of whole attribute for address_space

rsmith added a comment. You also need to update the various other places that create `AttributedType`s (eg, template instantiation, ASTImporter, deserialization) to pass in the macro name. You can try removing the default argument from `ASTContext::getAttributedType` to find other places that

[PATCH] D43871: [modules] No longer include stdlib.h from mm_malloc.h.

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: lib/Headers/mm_malloc.h:42 extern "C" int posix_memalign(void **__memptr, size_t __alignment, size_t __size); +extern "C" void(free)(void *ptr); +extern "C"

[PATCH] D51329: [Attribute/Diagnostics] Print macro instead of whole attribute for address_space

rsmith added a comment. +@aaron.ballman for attributes-related changes. It would be easier and more precise to track the macro corresponding to an attribute in the `Parser` rather than in `Sema` (and stash it on the `ParsedAttr` for consumers such as `Sema` that want it). That way, we still ha

[PATCH] D50927: [Sema] Remove location from implicit capture init expr

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Sema/SemaLambda.cpp:1422-1424 auto Entity = InitializedEntity::InitializeLambdaCapture( Var->getIdentifier(), Field->getType(), Loc); I

[PATCH] D49244: Always search sysroot for GCC installs

rsmith added a comment. I think `-gcc-toolchain`, if specified, should simply be taken as the location of the GCC toolchain; we should never go looking for it anywhere else if `-gcc-toolchain` is specified. So I think the patch is not quite right as-is, as it also affects that case. I think the

[PATCH] D6260: Add -mlong-double-64 flag

rsmith added a comment. What about `-mlong-double-128`? It seems sensible to me to add support for that at the same time. It looks like this also needs to affect name mangling. As far as I can tell from some quick testing, on ppc64-linux-gnu GCC uses `e` for 64-bit `long double` and `g` for 12

[PATCH] D51190: [AttrDocs]: document gnu_inline function attribute

rsmith added inline comments. Comment at: include/clang/Basic/AttrDocs.td:3514-3516 +(possibly due to being referred to by function pointer), then no out of line +definition will be emitted (instead of c99's behaviour of always emitting an +out of line definition). --

[PATCH] D51329: [Attribute/Diagnostics] Print macro instead of whole attribute for address_space

rsmith added inline comments. Comment at: include/clang/AST/ASTContext.h:1428 + QualType equivalentType, + IdentifierInfo *AddressSpaceMacroII = nullptr); There is no reason to make this specific to addres

[PATCH] D50927: [Sema] Remove location from implicit capture init expr

rsmith added inline comments. Comment at: clang/lib/Sema/SemaLambda.cpp:1422-1424 auto Entity = InitializedEntity::InitializeLambdaCapture( Var->getIdentifier(), Field->getType(), Loc); InitializationKind InitKind = InitializationKind::CreateDirect(Loc, Loc, Loc); -

[PATCH] D50927: [Sema] Remove location from implicit capture init expr

rsmith added a comment. I would prefer that we use the location of the capture-default as the location of all the implicitly captured fields, rather than using an invalid location. Comment at: clang/lib/Sema/SemaDecl.cpp:10689 - S.DiagRuntimeBehavior(DRE->getBeginLoc(),

[PATCH] D51229: [Sema/Attribute] Make types declared with address_space an AttributedType

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: lib/Sema/SemaType.cpp:5600-5605 if (DependentAddressSpaceTypeLoc DASTL = CurrTL.getAs()) { fillDependentAddressSpaceTypeLoc(DASTL, D.getT

[PATCH] D51240: Add a flag to remap manglings when reading profile data information.

rsmith added a comment. In https://reviews.llvm.org/D51240#1213179, @davidxl wrote: > Can you split the patch into two? One for sample PGO and one for > instrumentation. I've split the patch up thusly: https://reviews.llvm.org/D51246 is the common infrastructure shared by sample PGO and inst

[PATCH] D51240: Add a flag to remap manglings when reading profile data information.

rsmith updated this revision to Diff 162505. rsmith added a comment. Herald added a subscriber: hiraditya. (Tweak diff so phabricator shows clang/ and llvm/ prefixes on filenames and add more context.) https://reviews.llvm.org/D51240 Files: clang/docs/ReleaseNotes.rst clang/docs/UsersManua

[PATCH] D51240: Add a flag to remap manglings when reading profile data information.

rsmith created this revision. rsmith added reviewers: davidxl, tejohnson, dlj, erik.pilkington. Herald added subscribers: dexonsmith, steven_wu, mgorny, mehdi_amini. This can be used to preserve profiling information across codebase changes that have widespread impact on mangled names, but across

[PATCH] D51229: [Sema/Attribute] Make types declared with address_space an AttributedType

rsmith added inline comments. Comment at: lib/Sema/SemaType.cpp:5816 +if (!T.isNull()) { + if (!T->getAs()) { +ASTContext &Ctx = S.Context; We should also create the AttributedType to track the source syntax in the dependent case. If we don't, t

[PATCH] D51229: [Sema/Attribute] Make types declared with address_space an AttributedType

rsmith added inline comments. Comment at: lib/AST/TypePrinter.cpp:1430-1431 + if (T->getAttrKind() == attr::AddressSpace) +return; + Please justify why this is appropriate with a comment. Comment at: lib/AST/TypePrinter.cpp:1501-1502 + +

[PATCH] D51157: [x86/SLH] Add a real Clang flag and LLVM IR attribute for Speculative Load Hardening.

rsmith accepted this revision. rsmith added inline comments. Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1150 + Opts.SpeculativeLoadHardening = + Args.hasFlag(OPT_mspeculative_load_hardening, You can just use `hasArg(OPT_mspeculative_load_harden

[PATCH] D51157: [x86/SLH] Add a real Clang flag and LLVM IR attribute for Speculative Load Hardening.

rsmith added inline comments. Comment at: clang/include/clang/Driver/Options.td:2004-2007 +def mspeculative_load_hardening : Flag<["-"], "mspeculative-load-hardening">, + Group, Flags<[CoreOption,CC1Option]>; +def mno_speculative_load_hardening : Flag<["-"], "mno-speculative-lo

[PATCH] D49511: [Sema/Attribute] Check for noderef attribute

rsmith added inline comments. Comment at: lib/Parse/ParseExpr.cpp:1126 + +Actions.StartCheckingNoDeref(); + leonardchan wrote: > rsmith wrote: > > This parser-driven start/stop mechanism will not work in C++ templates. > > Instead, can you remove the "start"

[PATCH] D51011: [Preprocessor] raise gcc compatibility macros.

rsmith added a comment. Another possibility would be to add a command-line argument to set the version of GCC that clang pretends to be. We have a similar mechanism for our MSVC compatibility mode already. Repository: rC Clang https://reviews.llvm.org/D51011 _

[PATCH] D51011: [Preprocessor] raise gcc compatibility macros.

rsmith added a comment. We are not fully compatible with any version of GCC later than 4.2, as we do not implement `__builtin_va_arg_pack` / `__builtin_va_arg_pack_len`. These builtins are used by the glibc headers if we claim to be GCC >= 4.3 (at least according to http://clang.llvm.org/docs/

[PATCH] D50994: Add a new flag and attributes to control static destructor registration

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Seems fine to me, once you add the test for always_destroy + no_destroy. Comment at: clang/include/clang/Basic/LangOptions.def:311 +LANGOPT(RegisterStaticDestructors, 1, 1,

[PATCH] D50662: Add dump() method for SourceRange

rsmith added inline comments. Comment at: lib/Basic/SourceLocation.cpp:89-93 + B.print(OS, SM); + OS << ",\n "; + E.print(OS, SM); + OS << "]\n"; +} It would seem somewhat more natural to dump `SourceRange`s the same way we dump them in `-ast-dump`. (That is

[PATCH] D50994: Add a new flag and attributes to control static destructor registration

rsmith added inline comments. Comment at: clang/include/clang/AST/Decl.h:1472 + /// Do we need to emit an exit-time destructor for this variable? + bool isNoDestroy(const ASTContext &) const; jfb wrote: > This is only valid for static variables, right? It's p

<    12   13   14   15   16   17   18   19   20   21   >