[PATCH] D67982: [ObjC] Add some additional test cases around pointer conversions.

jyknight created this revision. jyknight added a reviewer: rjmccall. Herald added a project: clang. Herald added a subscriber: cfe-commits. This is especially important for Objective-C++, which is entirely missing this testing at the moment. This annotates with "FIXME" the cases which I change in

[PATCH] D67983: [ObjC] Diagnose implicit type coercion from ObjC 'Class' to object pointer types.

jyknight created this revision. jyknight added a reviewer: rjmccall. Herald added a project: clang. Herald added a subscriber: cfe-commits. For example, in Objective-C mode, the initialization of 'x' in: @implementation MyType + (void)someClassMethod { MyType *x = self; } @end is cor

[PATCH] D67983: [ObjC] Diagnose implicit type coercion from ObjC 'Class' to object pointer types.

jyknight added a comment. Note that the test-case diffs are on top of https://reviews.llvm.org/D67982, which I split out to make the actual change in behavior in this commit clearer. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67983/new/ https:/

[PATCH] D67982: [ObjC] Add some additional test cases around pointer conversions.

jyknight added a comment. (See https://reviews.llvm.org/D67983 for the proposed behavior change.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67982/new/ https://reviews.llvm.org/D67982 ___ cfe-commit

[PATCH] D67573: Fix __atomic_is_lock_free's return type.

jyknight added a comment. Ping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67573/new/ https://reviews.llvm.org/D67573 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/c

[PATCH] D68410: [AttrDocs] document always_inline

jyknight added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:4399-4400 + let Content = [{ +Hint that inline substitution should be attempted when optimizations are +disabled. Does not guarantee that inline substitution actually occurs. +}];

[PATCH] D67847: [Support] make report_fatal_error `abort` instead of `exit`

jyknight added a comment. The `abort()` function raises SIGABRT, for which the default behavior is to trigger a coredump. Do we actually want that behavior? Either `_exit()` (long available extension, which lld already uses) or `quick_exit()` (the new C standard way) seem possibly preferable?

[PATCH] D47894: [clang]: Add support for "-fno-delete-null-pointer-checks"

jyknight added a comment. In https://reviews.llvm.org/D47894#1158811, @manojgupta wrote: > @efriedma @jyknight Does the change match your expectations where warnings > are still generated but codeGen does not emit nonnull attribute? Yes, this seems sensible IMO. Comment at:

[PATCH] D47894: [clang]: Add support for "-fno-delete-null-pointer-checks"

jyknight accepted this revision. jyknight added a comment. This revision is now accepted and ready to land. Please refer to the commit for the LLVM half of the change in the commit message for this. LGTM, other than some minor suggestions for the help texts. Comment at: docs/

[PATCH] D28213: [Frontend] Correct values of ATOMIC_*_LOCK_FREE to match builtin

jyknight reopened this revision. jyknight added a comment. This revision is now accepted and ready to land. The close was due to phabricator problem, reopening. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D28213/new/ https://reviews.llvm.org/D2821

[PATCH] D67982: [ObjC] Add some additional test cases around pointer conversions.

This revision was automatically updated to reflect the committed changes. Closed by commit rG1c982af05997: [ObjC] Add some additional test cases around pointer conversions. (authored by jyknight). Changed prior to commit: https://reviews.llvm.org/D67982?vs=221586&id=225439#toc Repository: rG

[PATCH] D67983: [ObjC] Diagnose implicit type coercion from ObjC 'Class' to object pointer types.

This revision was automatically updated to reflect the committed changes. Closed by commit rGccc4d83cda16: [ObjC] Diagnose implicit type coercion from ObjC 'Class' to object pointer… (authored by jyknight). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.or

[PATCH] D67983: [ObjC] Diagnose implicit type coercion from ObjC 'Class' to object pointer types.

jyknight added a comment. In D67983#1723376 , @thakis wrote: > After this, Class can no longer be used as a key type in an Obj-C dictionary > literal. Is that intentional? Such code was already an on by default incompatible-pointer-types warning in Obj

[PATCH] D67983: [ObjC] Diagnose implicit type coercion from ObjC 'Class' to object pointer types.

jyknight added a comment. In D67983#1723681 , @rjmccall wrote: > We could probably do a quick check to see if the class informally conforms to > the protocol. `+copyWithZone` seems to be marked unavailable in ARC; not > sure if that would cause problems

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

jyknight added a comment. In the commit message, you refer to the preferred alignemtn as the "true" alignment, but that's misleading. As discussed on the mailing list thread, it's not true alignment at all. Comment at: clang/include/clang/AST/RecordLayout.h:74 + /// The maxi

[PATCH] D80225: [Driver] Recognize -fuse-ld={bfd, gold, lld} but don't prepend "ld." or "ld64." for other values

jyknight added a comment. It's worrying to me that there number of places in LLVM that at the exact argument value of "-fuse-ld=". E.g. in the windows and PS4 toolchains. We already claim to support arbitrary values and full paths, but if you specify "-fuse-ld=/path/to/lld-link" on Windows toda

[PATCH] D80417: Fix Darwin 'constinit thread_local' variables.

jyknight created this revision. jyknight added a reviewer: rjmccall. Herald added a project: clang. Herald added a subscriber: cfe-commits. Unlike other platforms using ItaniumCXXABI, Darwin does not allow the creation of a thread-wrapper function for a variable in the TU of users. Because of this

[PATCH] D80417: Fix Darwin 'constinit thread_local' variables.

This revision was automatically updated to reflect the committed changes. jyknight marked an inline comment as done. Closed by commit rGaca3d067efe1: Fix Darwin 'constinit thread_local' variables. (authored by jyknight). Changed prior to commit: https://reviews.llvm.org/D80417?vs=265642&id=2665

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

jyknight added a comment. Why not have clang always emit atomicrmw for floats, and let AtomicExpandPass handle legalizing that into integer atomics if necessary, rather than adding a target hook in clang? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71726/new/ https://reviews.llvm.or

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

jyknight added a comment. In D71726#2165445 , @yaxunl wrote: > In D71726#2165424 , @jyknight wrote: > > > Why not have clang always emit atomicrmw for floats, and let > > AtomicExpandPass handle legalizing that int

[PATCH] D81313: Fix handling of constinit thread_locals with a forward-declared type.

jyknight created this revision. jyknight added a reviewer: rsmith. Herald added a project: clang. Herald added a subscriber: cfe-commits. ItaniumCXXABI::usesThreadWrapperFunction calls VarDecl::needsDestruction, which calls QualType::isDestructedType, which checks CXXRecordDecl::hasTrivialDestruct

[PATCH] D81432: Add #includes so that ROCm.h is compilable stand-alone.

jyknight added a comment. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81432/new/ https://reviews.llvm.org/D81432 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/c

[PATCH] D81772: Reduce -Wregister from DefaultError to a default-on warning.

jyknight created this revision. jyknight added a reviewer: rsmith. Herald added a project: clang. Herald added a subscriber: cfe-commits. There is a lot of C++ code in the wild still using 'register', which will become broken if we switch the default compilation mode to C++17. A default-on warning

[PATCH] D83015: [Driver] Add -fld-path= and deprecate -fuse-ld=/abs/path and -fuse-ld=rel/path

jyknight added a comment. BTW, I just noticed recently that we have a -mlinker-version= flag, too, which is only used on darwin at the moment. That's another instance of "we need to condition behavior based on what linker we're invoking", but in this case, between multiple versions of apple's l

[PATCH] D82777: Clang Driver: Use Apple ld64's new @response-file support.

jyknight created this revision. jyknight added reviewers: steven_wu, arphaman. Herald added subscribers: cfe-commits, dexonsmith. Herald added a project: clang. jyknight added a child revision: D82782: Clang Driver: refactor support for writing response files to be specified at Command creation, r

[PATCH] D82782: Clang Driver: refactor support for writing response files to be specified at Command creation, rather than as part of the Tool.

jyknight created this revision. jyknight added reviewers: rnk, arphaman, steven_wu. Herald added subscribers: cfe-commits, sstefan1, kerbowa, luismarques, apazos, sameer.abuasal, pzheng, s.egerton, lenary, Jim, mstorsjo, jocewei, PkmX, dexonsmith, the_o, brucehoult, MartinMosbeck, rogfer01, edwar

[PATCH] D82777: Clang Driver: Use Apple ld64's new @response-file support.

This revision was automatically updated to reflect the committed changes. Closed by commit rG381df1653c92: Clang Driver: Use Apple ld64's new @response-file support. (authored by jyknight). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82777/new/ ht

[PATCH] D82782: Clang Driver: refactor support for writing response files to be specified at Command creation, rather than as part of the Tool.

This revision was automatically updated to reflect the committed changes. Closed by commit rG4772b99dffec: Clang Driver: refactor support for writing response files to be specified at… (authored by jyknight). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.

[PATCH] D66831: [ObjC] Fix type checking for qualified id block parameters.

jyknight added a comment. Hi, it seems that this change is incompatible with the `[NSItemProvider loadItemForTypeIdentifier:options:completionHandler:]` method's expected (but extremely weird and unusual!) usage. Per https://developer.apple.com/documentation/foundation/nsitemprovider/1403900-l

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

jyknight added a comment. In D71726#2182667 , @tra wrote: >> If a target would like to treat single and double fp atomics as unsupported, >> it can override the default behavior in its own TargetInfo. I really don't think this should be a target option a

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

jyknight added a comment. Oh, one more note, C11 has -- and clang already supports -- `_Atomic long double x; x += 4;` via lowering to a cmpxchg loop. Now that we have an LLVM IR representation for atomicrmw fadd/fsub, clang should be lowering the _Atomic += to that, too. (Doesn't need to be in

[PATCH] D66831: [ObjC] Fix type checking for qualified id block parameters.

jyknight added a comment. In D66831#2019125 , @vsapsai wrote: > Agree that is a mistake in `NSItemProvider` API. The solution I offer is not > to revert the change but to add cc1 flag > `-fcompatibility-qualified-id-block-type-checking` and to make the d

[PATCH] D79511: [ObjC] Add compatibility mode for type checking of qualified id block parameters.

jyknight added inline comments. Comment at: clang/test/SemaObjC/block-type-safety.m:170 +genericBlockWithParam = blockWithParam; +blockWithParam = genericBlockWithParam; // expected-error {{incompatible block pointer types assigning to 'void (^)(NSAllArray *)' from 'void

[PATCH] D79511: [ObjC] Add compatibility mode for type checking of qualified id block parameters.

jyknight accepted this revision. jyknight added a comment. This revision is now accepted and ready to land. It looks like you didn't squash your two commits before uploading, so the diff for review now only includes the changes for the comment, not the complete patch. Other than needing to squas

[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] 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] 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 belie

[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 belie

[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 cfe-commits@lists.llvm.or

[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 special-casi

[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 > h

[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] 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 a

[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 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 permit

[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: https://reviews.llvm.org/D120159?vs=41

[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] 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 decl

[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 someth

[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 th

[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 cfe-commits@lists.

[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 f

[PATCH] D106577: [clang] Define __STDC_ISO_10646__

jyknight added a comment. I'm not sure we should be populating this. The _value_ is determined by what libc supports, so it probably needs to be left up to libc to define it. In glibc, this define is set by the file /usr/include/stdc-predef.h, which GCC implicitly includes into all TUs whether

[PATCH] D106577: [clang] Define __STDC_ISO_10646__

jyknight added a comment. In D106577#2897588 , @aaron.ballman wrote: > In D106577#2897522 , @jyknight > wrote: > >> I'm not sure we should be populating this. >> >> The _value_ is determined by what libc support

[PATCH] D106577: [clang] Define __STDC_ISO_10646__

jyknight added a comment. Even after the more recent discussion, I still think my initial message was incorrect, and that the compiler should be defining this macro itself, as proposed in this patch. Note that my confusion was not that the macro being defined or not was dependent on libc behavi

[PATCH] D106577: [clang] Define __STDC_ISO_10646__

jyknight added a comment. In D106577#2901654 , @rsmith wrote: > I think this patch as it stands would cause problems with libc > implementations that put their `#define` somewhere other than than > `stdc-predef.h` (eg, older versions of glibc that put i

[PATCH] D106577: [clang] Define __STDC_ISO_10646__

jyknight added a comment. Perhaps a reasonable path forward here to address the BSD issue can be to add a targetinfo method: /* Returns true if the expected encoding of wchar_t changes at runtime depending on locale for this target. Note that clang always encodes wide character liter

[PATCH] D106577: [clang] Define __STDC_ISO_10646__

jyknight added a comment. In D106577#2904960 , @rsmith wrote: > One benefit we don't get with this approach is providing the right value for > the macro (without paying the cost of always including `stdc-predefs.h`). What do you mean by "right value", t

[PATCH] D106577: [clang] Define __STDC_ISO_10646__

jyknight added a comment. BTW, looks like the standard wording came from: http://www.open-std.org/jtc1/sc22/wg14/www/docs/dr_273.htm which indeed seems to suggest that the intent was to: 1. ensure that WCHAR_MAX is at least the maximum character actually defined so far by the standard (which in

[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] 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] 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 https

[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 an

[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: clang/lib/CodeGen/CGStmt.cpp:

[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] 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] 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 a

[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 i

[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] 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 af

[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. > > gcc

[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] 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] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

jyknight added a comment. In D118804#3292185 , @ychen wrote: > I don't see why the patch is wrong... It uses the target/platform-specific > `NewAlign`. If the platform allows customized memory allocation that assumes > weak alignment, it should set the

[PATCH] D110869: [X86] Implement -fzero-call-used-regs option

jyknight added a comment. In D110869#3295477 , @nathanchance wrote: > Rather interesting function to have problems with as a result of this patch > but it seems like this function is being used in a very specific way further > down the file with the `_

[PATCH] D110869: [X86] Implement -fzero-call-used-regs option

jyknight added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:6271 +This attribute, when attached to a function, causes the compiler to zero a +subset of all call-used registers before the function returns. It's used to +increase program security by either mit

[PATCH] D113620: Skip exception cleanups when the innermost scope is EHTerminateScope.

jyknight added a comment. Found the problem, tweaking a test-case; will commit shortly. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113620/new/ https://reviews.llvm.org/D113620 ___ cfe-commits mailing

[PATCH] D113620: Skip exception cleanups when the innermost scope is EHTerminateScope.

jyknight added a comment. Should be fixed by rGcaa1ebde70673ddb7124a0599ba846362a1f8b1e . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113620/new/ https://reviews.llvm.org/D1136

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

jyknight updated this revision to Diff 406478. jyknight added a comment. Rebase, and preserve (and modify) test-case. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118804/new/ https://reviews.llvm.org/D118804 Files: clang/include/clang/Basic/Tar

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

jyknight added a comment. In D118804#3301746 , @Lapenkov wrote: > Is it hard to retrofit this diff into LLVM 13? It would be easy to backport this change into LLVM 13, but the problem is that there are no more releases planned on the LLVM 13 branch. R

[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 is

[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 C

[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 &Attrs) { +llvm::AttributeList NewAttrs = Attrs; +if (AA) We do need to fallback to an assume for "if(CGF.SanOpts.has(Sa

[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] 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 t

[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 serge-sans-p

[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 mu

[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 tra

[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 this

[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 Marsha

[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 fig

[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 [-Wdeprecated-non-proto

[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 c

[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] 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] 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 construct

[PATCH] D57450: [RISCV] Set MaxAtomicInlineWidth and MaxAtomicPromoteWidth for RV32/RV64 targets with atomics

jyknight added a comment. That GCC and Clang differ in handling of Atomics is a really unfortunate, longstanding issue. See https://bugs.llvm.org/show_bug.cgi?id=26462 For RISCV, perhaps it's not yet too late to have the RISCV psABI actually specify a single ABI for C11 _Atomic which all compi

[PATCH] D57450: [RISCV] Set MaxAtomicInlineWidth and MaxAtomicPromoteWidth for RV32/RV64 targets with atomics

jyknight added a comment. In D57450#1641190 , @lenary wrote: > @jyknight I hear where you're coming from. I'll see what I can do about the > psABI document. > > In that ticket, it's mentioned that the Darwin ABI explicitly says that > non-power-of-two at

[PATCH] D66822: Hardware cache line size builtins

jyknight added a comment. numbers for cacheline size. In D66822#1647664 , @zoecarver wrote: > > Passing-by remark: i'm not sure it is possible to **guarantee** that this > > will be always correct and that no ABI break will happen. > > You're right. I sh

[PATCH] D34158: For standards compatibility, preinclude if the file is available

jyknight added a comment. In https://reviews.llvm.org/D34158#823316, @fedor.sergeev wrote: > Hmm... I tried this patch and now the following worries me: > > - it passes -finclude-if-exists stdc-predef.h on all platforms (say, > including my Solaris platform that has no system stdc-predef.h) Ri

<    1   2   3   4   5   >