[PATCH] D156565: Diagnose use of VLAs in C++ by default

jyknight added inline comments. Comment at: clang/lib/Sema/SemaType.cpp:2617 + } else if (getLangOpts().CPlusPlus) { +if (getLangOpts().CPlusPlus11 && IsStaticAssertLike(ArraySize, Context)) + VLADiag = getLangOpts().GNUMode aaron.ballman wrote: >

[PATCH] D156565: Diagnose use of VLAs in C++ by default

jyknight added inline comments. Comment at: clang/lib/Sema/SemaType.cpp:2617 + } else if (getLangOpts().CPlusPlus) { +if (getLangOpts().CPlusPlus11 && IsStaticAssertLike(ArraySize, Context)) + VLADiag = getLangOpts().GNUMode aaron.ballman wrote: >

[PATCH] D156565: Diagnose use of VLAs in C++ by default

jyknight added a comment. Overall looks good, just a few nits from looking things over. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:143 +def ext_vla_cxx : ExtWarn< + "variable length arrays are a Clang extension">, InGroup; +def ext_vla_cxx_in_gnu_mode :

[PATCH] D153156: [Clang] CWG1473: do not err on the lack of space after operator""

jyknight reopened this revision. jyknight added a comment. This revision is now accepted and ready to land. Reopening the review since it ended up reverted, but I do think we DO want to have at least some of the changes proposed here (and/or from the pending D158372

[PATCH] D159250: [X86][RFC] Add new option `-m[no-]evex512` to disable ZMM and 64-bit mask instructions for AVX512 features

jyknight added a comment. In D159250#4633530 , @pengfei wrote: > In D159250#4631786 , @RKSimon wrote: > >> Would it be possible to add function multiversioning tests to ensure the >> evex512 attribute would work

[PATCH] D153156: [Clang] CWG1473: do not err on the lack of space after operator""

jyknight added a comment. In D153156#4599595 , @rZhBoYao wrote: > What if a programmer is really trying to call operator""b here (albeit > ill-formed) Because that code is ill-formed, Clang diagnosed it with an error by default. Isn't that preferable

[PATCH] D153156: [Clang] CWG1473: do not err on the lack of space after operator""

jyknight added a comment. > The diagnostic behavior is correct. MYTHING doesn't get expanded until phase > 4 (http://eel.is/c++draft/lex.phases#1.4), so this appears as "ONE"MYTHING as > a single preprocessor token: > https://eel.is/c++draft/lex.ext#nt:user-defined-string-literal and that

[PATCH] D157793: [Headers] Add missing __need_ macros to stdarg.h

jyknight added a comment. I'm a bit confused by this change vs its description. It looks like stdarg already supported `__need___va_list`, which is what you said Apple needs. Does Apple //also// require the other __need_va_copy/etc, too? If not, why add support for them? (I think we should

[PATCH] D141918: WIP: [Clang] Emit 'unwindabort' when applicable.

jyknight added a comment. In D141918#4566838 , @smeenai wrote: > $ clang -std=c++20 -O2 -c crash.cpp > cannot use musttail with unwindabort Thanks for the report. We were missing a check of `CI.isUnwindAbort()` in CoroSplit.cpp's `shouldBeMustTail()`

[PATCH] D155387: [Clang] Fix member lookup so that we don't ignore ambiguous lookups in some cases

jyknight added a comment. In D155387#4557834 , @hctim wrote: > I found an issue with building Android using this patch. I've reduced it down > to the following problem where the evaluation of the `std::visit` is believed > to be non-exhaustive, but it

[PATCH] D141918: WIP: [Clang] Emit 'unwindabort' when applicable.

jyknight updated this revision to Diff 546613. jyknight edited the summary of this revision. jyknight added a comment. Rebase patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141918/new/ https://reviews.llvm.org/D141918 Files:

[PATCH] D156286: [docs] Bump minimum GCC version to 7.5

jyknight added a comment. Given that folks have successfully tested with GCC 7.4, and the lateness of the change in the release process for LLVM 17, I think it'd be better to require GCC 7.4 (the earliest that actually works), instead of increasing the requirement to 7.5. Repository: rG

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

jyknight added a comment. As I posted on the RFC thread, I think we have a viable alternative solution to address the original motiving use-case. One might potentially still make a case for implementing the `-fno-coroutines` flag for GCC compatibility, but given the concerns raised -- and that

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

jyknight added inline comments. Comment at: clang/lib/Sema/JumpDiagnostics.cpp:658 +if (auto *G = dyn_cast(Jump)) { + for (AddrLabelExpr *L : G->labels()) { efriedma wrote: > nickdesaulniers wrote: > > efriedma wrote: > > > nickdesaulniers wrote: > >

[PATCH] D121629: clang: also check alloc_alignment claims in return

jyknight added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:3684 + const AllocAlignAttr *AA = CurCodeDecl->getAttr(); + if (SanOpts.has(SanitizerKind::Alignment) && AA) { It may be nice to also verify the alignment required by an AssumeAlignAttr

[PATCH] D123085: CGExprCXX: emit allocptr attributes for operator delete

jyknight added inline comments. Comment at: clang/lib/CodeGen/CGExprCXX.cpp:1333 +CalleeDecl->getOverloadedOperator() == OO_Delete) { + CallOrInvoke->addParamAttr(0, llvm::Attribute::AllocatedPointer); +} Perhaps we should add this attribute to

[PATCH] D152401: CGExprCXX: emit family and allockind attributes

jyknight added a comment. > As a result, clang/llvm should correctly > optimize custom allocators like this without needing specialized > knowledge going forward. Not exactly: we still need knowledge of this in //clang// (`isReplaceableGlobalAllocationFunction` was taught that the variant with

[PATCH] D148700: [clang] Add support for “regular” keyword attributes

jyknight added a comment. In D148700#4418767 , @rsandifo-arm wrote: > Hi @jyknight , @rsmith > > Do you have any more thoughts on the above? Quick version is: > > 1. Is it OK to have `[[…]]` attributes in the `arm` namespace that affect > semantics?

[PATCH] D148700: [clang] Add support for “regular” keyword attributes

jyknight added a comment. > This makes ``CXX11`` and ``C2x`` spellings > unsuitable for attributes that affect the type system, that change the > binary interface of the code, or that have other similar semantic meaning. Yes, standard attributes aren't supposed to be used for things which affect

[PATCH] D150226: [Clang] Remove ability to downgrade warning on the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

jyknight added a comment. In D150226#4400782 , @rupprecht wrote: > As a general question/feature request: is there a way to have specific > warnings apply even for system headers? It would be nice if I could check > what breaks when by adding

[PATCH] D150226: [Clang] Remove ability to downgrade warning on the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

jyknight added a comment. When looking for errors in existing codebases, don't forget that this diagnostic is currently suppressed by default in system headers. So this patch is moving from "no diagnostics for code in system headers" to "unconditional hard error in system headers". Just

[PATCH] D144999: [RFC][MC][MachO]Only emits compact-unwind format for "canonical" personality symbols. For the rest, use DWARFs.

jyknight added a comment. I wonder if we actually need to define a clang frontend flag for this; I suspect nobody will ever want to specify it, since the only non-canonical personality clang will ever generate that changes behavior is the pure-C destructor-only personality,

[PATCH] D145416: [clang] model 'p' inline asm constraint as reading memory

jyknight requested changes to this revision. jyknight added a comment. This revision now requires changes to proceed. I believe this is abandoned, correct? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145416/new/ https://reviews.llvm.org/D145416

[PATCH] D145416: [clang] model 'p' inline asm constraint as reading memory

jyknight added a comment. Also, I note the doc says it's useful for `for “load address” and “push address” instructions` (note, "load address" means e.g. x86 "lea" instruction) -- which should NOT be dependent upon the value stored in the memory. The x86 backend actually uses a "Ts" constraint

[PATCH] D145416: [clang] model 'p' inline asm constraint as reading memory

jyknight added a comment. > ‘p’ in the constraint must be accompanied by address_operand as the predicate > in the match_operand. This predicate interprets the mode specified in the > match_operand as the mode of the memory reference for which the address would > be valid. How do you do that

[PATCH] D145416: [clang] model 'p' inline asm constraint as reading memory

jyknight added a comment. It looks to me from GCC that constraint 'p' is intended to be used internally, not for inline-asm. I question whether the kernel should be using it, and whether we should even implement it at all. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D122258: [MC] Omit DWARF unwind info if compact unwind is present where eligible

jyknight added a comment. > no-compact-unwind is particularly useful for newer x86_64 platforms: we don't > want to omit DWARF unwind for x86_64 in general due to possible backwards > compat issues, but we should make it possible for people to opt into this > behavior if they are only

[PATCH] D144889: [C2x] Support in freestanding

jyknight added a comment. > On the other hand, I think a not-insignificant number of users are interested > in freestanding environments for one-off/fun/experimental projects where ease > of access is more important than performance characteristics -- think: users > who are playing around with

[PATCH] D144889: [C2x] Support in freestanding

jyknight added a comment. In D144889#4156974 , @rsmith wrote: > Likely because of GCC's perspective on this, the set of C headers provided by > GCC, Clang, ICC, etc. has included the complete list of freestanding headers > and more or less no others,

[PATCH] D142925: [Clang] Improve error message for violations of -fmodules-decluse.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGab0116e2f05c: [Clang] Improve error message for violations of -fmodules-decluse. (authored by jyknight). Repository: rG LLVM Github Monorepo

[PATCH] D142925: [Clang] Improve error message for violations of -fmodules-decluse.

jyknight created this revision. jyknight added a reviewer: ChuanqiXu. Herald added a project: All. jyknight requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Now it reports the name of the indirectly-used module which is missing.

[PATCH] D139652: Add the thread sanitizer support for X86_64 WatchOS simulators

jyknight accepted this revision. jyknight added a comment. This revision is now accepted and ready to land. Thanks for the change. The description is a bit misleading, this would be better: Allow TSan in clang driver for X86_64 WatchOS simulator. It was already functional, and Apple's

[PATCH] D136707: [clang][Toolchains][Gnu] pass -gdwarf-* through to assembler

jyknight added a comment. This change has caused `clang -Wall -fdebug-default-version=5 -c -xc /dev/null -o /dev/null` to print the following warning, because it has moved the read of the argument into a conditional. `clang: warning: argument unused during compilation:

[PATCH] D134550: [Clang] Make Clang driver suggest '-Xclang' for CC1 options passed to the driver

jyknight added a comment. In D134550#3813269 , @aaron.ballman wrote: > Alternatively, perhaps those experimental options should be exposed from the > driver instead of being a cc1-only flag? IMO: yes. If we want end-users to use a particular flag, we

[PATCH] D134550: [Clang] Make Clang driver suggest '-Xclang' for CC1 options passed to the driver

jyknight added a comment. I //really// don't think we should have this behavior. The cc1 options are supposed to be an internal implementation detail. It's already a problem that the option name doesn't shout "hey I'm an internal interface with no stability guarantees! Don't use me!". Having

[PATCH] D133800: [Clang 15.0.1] Downgrade implicit int and implicit function declaration to warning only

jyknight accepted this revision. jyknight added a comment. This revision is now accepted and ready to land. Looks correct to me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133800/new/ https://reviews.llvm.org/D133800

[PATCH] D53847: [C++2a] P0634r3: Down with typename!

jyknight added a comment. >> Also, do you know if having posted this patch is agreement for licensing >> this code? Or do we need to get explicit agreement from the original author >> before committing a version of this? > > I've never seen that be an issue before, and I don't see enough in >

[PATCH] D129488: [Sema] Delay evaluation of std::source_location::current() in default arguments

jyknight added a comment. In D129488#3761478 , @ilya-biryukov wrote: > In D129488#3761398 , @jyknight > wrote: > >> I believe this should print `fn=14 fn2=14`. I don't see how we can get that >> by

[PATCH] D132944: [clang] cleanup -fstrict-flex-arrays implementation

jyknight added a comment. (please ignore the last comment, I sent it to the wrong review thread) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132944/new/ https://reviews.llvm.org/D132944 ___ cfe-commits mailing list

[PATCH] D129488: [Sema] Delay evaluation of std::source_location::current() in default arguments

jyknight added a comment. #include #include consteval int fn(std::source_location sl = std::source_location::current()) { return sl.line(); } consteval int fn2(int line = fn()) { return line; } int main() { printf("fn=%d fn2=%d\n", fn(), fn2()); } I

[PATCH] D132944: [clang] cleanup -fstrict-flex-arrays implementation

jyknight added a comment. #include #include consteval int fn(std::source_location sl = std::source_location::current()) { return sl.line(); } consteval int fn2(int line = fn()) { return line; } int main() { printf("fn=%d fn2=%d\n", fn(), fn2()); } I

[PATCH] D132944: [clang] cleanup -fstrict-flex-arrays implementation

jyknight added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:910 if (FD->getParent()->isUnion()) -return StrictFlexArraysLevel < 2; +return true; RecordDecl::field_iterator FI( This is a functional change (which is good,

[PATCH] D129488: [Sema] Delay evaluation of std::source_location::current() in default arguments

jyknight added a comment. I'm confused about this new strategy of special-casing `source_location::current()`. Isn't it wrong to eagerly evaluate _other_ calls in default args, as well? ISTM that source_location is simply _exposing_ the bug in where we evaluate these expressions, but that it's

[PATCH] D129833: Use @llvm.threadlocal.address intrinsic to access TLS

jyknight added a comment. Note that this change caused LLVM to no longer be aware that a TLS variable cannot be NULL. Thus, code like: __thread int x; int main() { int* y = return *y; } built with `clang -O -fsanitize=null` emits a null-pointer check, when it wouldn't

[PATCH] D124435: [X86] Always extend the integer parameters in callee

jyknight added a comment. In D124435#3697259 , @rjmccall wrote: > I know what you're saying, but I don't think it matches any model of how > programmers use command line flags. You're imagining that a programmer sits > down and considers all of their

[PATCH] D130058: [Clang] Diagnose ill-formed constant expression when setting a non fixed enum to a value outside the range of the enumeration values

jyknight added a comment. I've filed a bug against Boost MPL, https://github.com/boostorg/mpl/issues/69 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130058/new/ https://reviews.llvm.org/D130058 ___ cfe-commits mailing list

[PATCH] D124435: [X86] Always extend the integer parameters in callee

jyknight added a comment. In D124435#3696862 , @rjmccall wrote: > That is an interesting idea. I like that it integrates this into > `-fclang-abi-compat`. The way that `-mno-conservative-small-integer-abi` > ends up meaning opposite things based on

[PATCH] D124435: [X86] Always extend the integer parameters in callee

jyknight added a comment. Thinking more about the naming issue, I think the real issue is that the proposed options are an implementation detail, not what we should expose to users. I realize that the non-boolean option was proposed by rjmmccall before, but I'd suggest that we go back to

[PATCH] D129802: [DRAFT] Implementing new atomic orderings in LLVM and generate barriers for legacy __sync builtins. Support corresponding memory model in outline atomics as well.

jyknight added a comment. In D129802#3654943 , @Wilco1 wrote: > The general requirement is that inline and outline atomics have identical > behaviour, and that GCC and LLVM emit the same sequences. I agree sync is > badly documented, so it's hard to

[PATCH] D126864: [clang] Introduce -fstrict-flex-arrays= for stricter handling of flexible arrays

jyknight added a comment. Sorry if I was unclear -- I did intend that the remaining minor open comments be addressed before commit. I've quoted them again here for clarity. Please address in a follow-up, thanks! Comment at: clang/include/clang/Driver/Options.td:1143

[PATCH] D129802: [DRAFT] Implementing new atomic orderings in LLVM and generate barriers for legacy __sync builtins. Support corresponding memory model in outline atomics as well.

jyknight added a comment. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65697 is the relevant discussion thread that added these to GCC. I personally never found it very convincing, even back in 2015 when they first made the change. And, now, 7 years later, I'd be even more reluctant to add

[PATCH] D126864: [clang] Introduce -fstrict-flex-arrays= for stricter handling of flexible arrays

jyknight added a comment. Just to move things forward here: I propose to continue and land this patch without mode 3 (there were only a couple comments left to address for that), and continue the discussion about whether to add mode 3 elsewhere. (I don't mind where, KSPP, gcc, or llvm issue

[PATCH] D126864: [clang] Introduce -fstrict-flex-arrays= for stricter handling of flexible arrays

jyknight added a comment. In D126864#3645994 , @kees wrote: > I should clarify: I still need the =3 mode. Since sizeof([0]) == 0 and > sizeof([]) == error, they are being treated differently already by the > compiler causing bugs in Linux. The kernel

[PATCH] D126864: [clang] Introduce -fstrict-flex-arrays= for stricter handling of flexible arrays

jyknight added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:906 // member, only a T[0] or T[] member gets that treatment. + // Under StrictFlexArraysLevel, obey c99+ that disallows FAM in union, see + // C11 6.7.2.1 §18

[PATCH] D126864: [clang] Introduce -fstrict-flex-arrays= for stricter handling of flexible arrays

jyknight added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:892 + return false; if (CAT->getSize().ugt(1)) return false; Similar to SemaChecking below, could use a comment like: FIXME: While the default -fstrict-flex-arrays=0

[PATCH] D126864: [clang] Introduce -fstrict-flex-arrays= for stricter handling of flexible arrays

jyknight added inline comments. Comment at: clang/docs/ClangCommandLineReference.rst:2644 + +Control which arrays are considered as flexible arrays members. +can be 1 (array of size 0, 1 and undefined are considered), 2 (array of size 0 Docs should also mention

[PATCH] D126864: [clang] Introduce -fstrict-flex-arrays= for stricter handling of flexible arrays

jyknight added a comment. As I commented on https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836#c32, > It doesn't make sense to have a mode in which `int array[0]` is accepted but > is not a flex array. > Either that should be a compilation error (as the standard specifies), or it > should be

[PATCH] D127545: Remove Itanium EH ABI workaround to allow modifying a pointer caught by-reference.

jyknight added a comment. (Note: if folks agree with this direction, I'll file a new bug for the newly broken behavior -- even though we probably cannot actually fix it -- and link it from the commit.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D127545: Remove Itanium EH ABI workaround to allow modifying a pointer caught by-reference.

jyknight created this revision. jyknight added reviewers: rjmccall, nikic. Herald added a project: All. jyknight requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Pointers will now always be caught by-value. The workaround was originally

[PATCH] D125919: Drop qualifiers from return types in C (DR423)

jyknight added a comment. In D125919#3556754 , @rsmith wrote: > That assumption does not hold. Given `struct A { char c[3]; };`, `struct A` > has size 3 and align 1, but `_Atomic struct A` has size 4 and align 4 across > many (perhaps all?) of our

[PATCH] D125291: Introduce @llvm.threadlocal.address intrinsic to access TLS variable (1/3)

jyknight added a comment. So, I've been spending some significant time looking into this. I found that I couldn't really review this change for correctness, because I find it basically impossible to figure out whether the intrinsic calls have actually been added to all the right places in

[PATCH] D126170: C++ DR2394: Const-default-constructible for members.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG997b072e10d2: C++ DR2394: Const-default-constructible for members. (authored by jyknight). Changed prior to commit:

[PATCH] D125814: Improve the strict prototype diagnostic behavior

jyknight accepted this revision. jyknight added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Sema/SemaDecl.cpp:3959 + // it's a user-visible declaration. There is one exception to this: + // when the new declaration is

[PATCH] D126324: [clang] Allow const variables with weak attribute to be overridden

jyknight added a comment. In D126324#3536785 , @aaron.ballman wrote: > The changes so far look sensible, but I think we should add some more tests > for a few situations. 1) Using a const weak symbol as a valid initializer > should be diagnosed (with

[PATCH] D125814: Improve the strict prototype diagnostic behavior

jyknight added a comment. Good improvement, but an additional change to wording for just the zero-arg non-prototype function declaration case could help a lot. The fact that zero-arg it's only being warned about because of the "...because of" note isn't particularly clear -- especially when

[PATCH] D126170: C++ DR2394: Const-default-constructible for members.

jyknight created this revision. jyknight added reviewers: aaron.ballman, rsmith. Herald added a project: All. jyknight requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Const class members may be initialized with a defaulted default

[PATCH] D125773: [Driver] Do not auto-enable header modules with -std=c++20

jyknight added a comment. In D125773#3523459 , @rsmith wrote: > Header modules are part of the C++20 standard (where they are called "header > units"), and module maps are an intended way for Clang to provide this > functionality in C++20 mode. I don't

[PATCH] D125814: Fix strict prototype diagnostic wording for definitions

jyknight added a comment. This confuses me. Looking at behavior with default flags: We won't emit a -Wdeprecated-non-prototype warning for `int foo();`, until we subsequently find `int foo(int arg) { return 5; }`. Since we definitely have the context of what's going on at that point, in order

[PATCH] D124435: [X86] Always extend the integer parameters in callee

jyknight added a comment. I find the option names you have a bit confusing. I'd like to suggest calling them, instead: caller: Extend a small integer parameter in the caller; callee will assume it has already been extended. callee : Pass a small integer parameter directly in caller, extend in

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

jyknight added a comment. The warnings for this case aren't great: int foo(); int foo(int arg) { return 5; } results in: /tmp/test.c:1:5: warning: a function declaration without a prototype is deprecated in all versions of C and is not supported in C2x

[PATCH] D122983: [C11/C2x] Change the behavior of the implicit function declaration warning

jyknight accepted this revision. jyknight added a comment. Looks good to me, whichever way you go regarding rsmith's comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122983/new/ https://reviews.llvm.org/D122983 ___ cfe-commits mailing

[PATCH] D122983: [C11/C2x] Change the behavior of the implicit function declaration warning

jyknight added a comment. In D122983#3454406 , @aaron.ballman wrote: > In D122983#3452994 , @rsmith wrote: > >> I think we should just make this an error by default in C99 onwards; > > Out of curiosity -- do you

[PATCH] D123642: [clang codegen] Assume arguments of __atomic_* are aligned.

jyknight added a comment. In D123642#3450110 , @efriedma wrote: >> I disagree with this on principle -- IMO, it is basically a historical bug >> in GCC that it ignores the type alignment, and we should NOT try to match >> that -- it's dangerous. > >

[PATCH] D123642: [clang codegen] Assume arguments of __atomic_* are aligned.

jyknight added a comment. I disagree with this on principle -- IMO, it is basically a historical bug in GCC that it ignores the type alignment, and we should NOT try to match that -- it's dangerous. We ought to resolve the bug via other fixes: - As a workaround: add alignas(uint64_t) to the

[PATCH] D123627: Correctly diagnose prototype redeclaration errors in C

jyknight accepted this revision. jyknight added inline comments. Comment at: clang/test/Sema/warn-deprecated-non-prototype.c:64-70 // FIXME: we get two diagnostics here when running in pedantic mode. The first // comes when forming the function type for the definition, and the

[PATCH] D122983: [C11/C2x] Change the behavior of the implicit function declaration warning

jyknight added a comment. Looks reasonable overall. I've added a few specific comments to some tests, but in general, I think we should avoid adding -std=c99 to test-cases to workaround implicit decl issues, unless: - the test is explicitly testing the behavior of implicit decls (potentially

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

jyknight added inline comments. Comment at: clang/test/Sema/knr-def-call.c:15 +void f2(float); // expected-note{{previous declaration is here}} \ + expected-warning {{this function declaration with a prototype changes behavior in C2x because it is followed by

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

jyknight added inline comments. Comment at: clang/test/CodeGen/2009-06-01-addrofknr.c:8 -static int func(f) +static int func(f) // expected-warning {{this function declaration without a prototype is deprecated in all versions of C and changes behavior in C2x}} void *f;

[PATCH] D121556: [randstruct] Add randomize structure layout support

jyknight added inline comments. Comment at: clang/unittests/AST/RandstructTest.cpp:154-158 +#ifdef _WIN32 + const field_names Expected = {"lettuce", "bacon", "mayonnaise", "tomato"}; +#else + const field_names Expected = {"mayonnaise", "bacon", "tomato", "lettuce"}; +#endif

[PATCH] D105142: RFC: Implementing new mechanism for hard register operands to inline asm as a constraint.

jyknight added a comment. Herald added a subscriber: StephenFan. Herald added a project: All. What's the status of this? Did the GCC proposal go anywhere? I'd be happy to see this move forward if you're also pushing it on the GCC side. Comment at:

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

jyknight added a comment. Starting by looking at the test cases, I've got some suggestions on making the diagnostics a bit less confusing. Comment at: clang/test/CodeGen/2009-06-01-addrofknr.c:8 -static int func(f) +static int func(f) // expected-warning {{this function

[PATCH] D120159: [Clang] Implement __builtin_source_location.

jyknight added a comment. In D120159#3415112 , @kda wrote: > This appears to have broken buildbot: sanitizer-x86_64-linux-fast > > Validating if reversion clears it up. > > Then will revert. Fixed by 8f66f1371981bda1af1ca43d505e1bc5836b3e36

[PATCH] D120159: [Clang] Implement __builtin_source_location.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGd61487490022: [Clang] Implement __builtin_source_location. (authored by jyknight). Changed prior to commit:

[PATCH] D121328: Disable -Wmissing-prototypes for internal linkage functions that aren't explicitly marked "static"""

jyknight added a comment. Given that this is an error `error: functions cannot be declared in an anonymous struct` (thus you can assume that function declarations inside a struct imply that the struct MUST have a linkage name) struct { static int foo(); }; then I don't think there's

[PATCH] D120159: [Clang] Implement __builtin_source_location.

jyknight added a comment. Ping; any more comments? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120159/new/ https://reviews.llvm.org/D120159 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D119271: clang: emit allocalign to LLVM for alloc_align attributes

jyknight added inline comments. Comment at: clang/test/CodeGen/catch-alignment-assumption-attribute-alloc_align-on-function-variable.cpp:55 // CHECK-NEXT:%[[ALIGNMENT_RELOADED:.*]] = load i64, i64* %[[ALIGNMENT_ADDR]], align 8 - // CHECK-NEXT:

[PATCH] D118511: Add a warning for not packing non-POD members in packed structs

jyknight added a comment. In D118511#3371432 , @tstellar wrote: > I'm fine with reverting if you think this is the best solution. I just would > like to conclude soon so I can make the final release candidate. ISTM that reverting the ABI change in the

[PATCH] D120159: [Clang] Implement __builtin_source_location.

jyknight updated this revision to Diff 412087. jyknight marked an inline comment as done. jyknight added a comment. Fix and test __impl lookup within the definition of std::source_location. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120159/new/

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

jyknight added a comment. In D119051#3316026 , @dblaikie wrote: > Ah, looks like this is the existing > https://github.com/itanium-cxx-abi/cxx-abi/issues/66 If you're going to change the ABI, you might as well tackle the rest of the differences

[PATCH] D120159: [Clang] Implement __builtin_source_location.

jyknight marked 8 inline comments as done. jyknight added a comment. In D120159#3349069 , @aaron.ballman wrote: > Ah, okay, that's a good point. I was thinking this would show up in the AST > in places like: > > template > auto func() -> Ty; >

[PATCH] D120159: [Clang] Implement __builtin_source_location.

jyknight added a comment. In D120159#3341224 , @aaron.ballman wrote: > Btw, I think there may be some functionality missing for AST dumping, so I'd > like to see some additional tests for that. I'm not sure what test would actually show a problem (or

[PATCH] D120159: [Clang] Implement __builtin_source_location.

jyknight updated this revision to Diff 411811. jyknight marked 15 inline comments as done. jyknight added a comment. Update per review comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120159/new/ https://reviews.llvm.org/D120159 Files:

[PATCH] D120159: [Clang] Implement __builtin_source_location.

jyknight updated this revision to Diff 411692. jyknight added a comment. Minor tweaks to comments and docs. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120159/new/ https://reviews.llvm.org/D120159 Files: clang/docs/LanguageExtensions.rst

[PATCH] D119271: CGCall: also emit LLVM `allocalign` attribute when handling AllocAlign

jyknight added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:4605 + TryEmitAsCallSiteAttribute(const llvm::AttributeList ) { +llvm::AttributeList NewAttrs = Attrs; +if (AA) durin42 wrote: > jyknight wrote: > > We do need to fallback to an

[PATCH] D120159: [Clang] Implement __builtin_source_location.

jyknight created this revision. jyknight added reviewers: aaron.ballman, rsmith. Herald added subscribers: dexonsmith, arphaman, martong. Herald added a reviewer: shafik. jyknight requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. This

[PATCH] D119707: [C2x] Implement WG14 N2764 the [[noreturn]] attribute

jyknight added inline comments. Comment at: clang/lib/Headers/stdnoreturn.h:22 +/* Including the header file in C2x is also deprecated. */ +#warning "the '' header is deprecated in C2x" +#endif aaron.ballman wrote: > jyknight wrote: > > Would be nice to mention

[PATCH] D119707: [C2x] Implement WG14 N2764 the [[noreturn]] attribute

jyknight added inline comments. Comment at: clang/lib/Headers/stdnoreturn.h:19 +/* The noreturn macro is deprecated in C2x. */ +#pragma clang deprecated(noreturn) + Is this actually useful? Isn't it sufficient to have the include-time warning for this header?

[PATCH] D119271: CGCall: also emit LLVM `allocalign` attribute when handling AllocAlign

jyknight added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:4605 + TryEmitAsCallSiteAttribute(const llvm::AttributeList ) { +llvm::AttributeList NewAttrs = Attrs; +if (AA) We do need to fallback to an assume for

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG9545976ff160: Revert [Clang] Propagate guaranteed alignment for malloc and others (authored by jyknight). Repository: rG LLVM Github Monorepo

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

jyknight added a comment. In D118804#3302675 , @rsmith wrote: > I support this revert. Having received enough support, I'll go ahead and commit, and then propose backport to llvm 14 branch. But -- > - `malloc` always returns storage whose alignment

  1   2   3   4   5   >