[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

jfb created this revision. jfb added reviewers: JDevlieghere, bruno. Herald added subscribers: cfe-commits, dexonsmith, jkorous. Herald added a project: clang. The motivation for 'physical' mode in D58169 was pretty sensible, but it has one unfortunate side-effec

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

jfb marked an inline comment as done. jfb added inline comments. Comment at: clang/test/Driver/vfsmode.py:4 + +# UNSUPPORTED: system-windows + I'm not sure what the best way to test this on Windows would be, and without a machine handy I can't really test this :

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

jfb updated this revision to Diff 214291. jfb marked 6 inline comments as done. jfb added a comment. - Address comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65986/new/ https://reviews.llvm.org/D65986 Files: clang/include/clang/Driver/D

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

jfb added inline comments. Comment at: clang/lib/Driver/Driver.cpp:172 + case VFSMode::Unknown: +if (!this->VFS) { + LLVM_FALLTHROUGH; JDevlieghere wrote: > Is this really necessary? the `Driver` ctor takes a `VFS` parameter which some tools set (other

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

jfb updated this revision to Diff 214292. jfb added a comment. - Undo whitespaces change. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65986/new/ https://reviews.llvm.org/D65986 Files: clang/include/clang/Driver/Driver.h clang/include/clang/D

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

jfb updated this revision to Diff 214296. jfb marked an inline comment as done. jfb added a comment. - Address more comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65986/new/ https://reviews.llvm.org/D65986 Files: clang/include/clang/Dri

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

jfb marked an inline comment as done. jfb added inline comments. Comment at: clang/lib/Driver/Driver.cpp:172 + case VFSMode::Unknown: +if (!this->VFS) { + LLVM_FALLTHROUGH; JDevlieghere wrote: > jfb wrote: > > JDevlieghere wrote: > > > Is this really ne

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

jfb updated this revision to Diff 214298. jfb marked an inline comment as done. jfb added a comment. - Address more comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65986/new/ https://reviews.llvm.org/D65986 Files: clang/include/clang/Dri

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

jfb marked 2 inline comments as done. jfb added inline comments. Comment at: clang/test/Driver/vfsmode.py:4 + +# UNSUPPORTED: system-windows + labath wrote: > jfb wrote: > > I'm not sure what the best way to test this on Windows would be, and > > without a machi

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

jfb marked an inline comment as done. jfb added a comment. In D65986#1622447 , @sammccall wrote: > Tagging D62271 and @Bigcheese as this was > the change that switched the default in Driver. (My motivation for D58169 > <

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

jfb marked 4 inline comments as done. jfb added inline comments. Comment at: clang/include/clang/Driver/Options.td:2010 HelpText<"Overlay the virtual filesystem described by file over the real file system">; +def ivfsmode : Joined<["-"], "ivfsmode=">, Group, Flags<[CC1Option

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

jfb updated this revision to Diff 214397. jfb added a comment. - Document asserts. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65986/new/ https://reviews.llvm.org/D65986 Files: clang/include/clang/Driver/Driver.h clang/include/clang/Driver/O

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

jfb marked 2 inline comments as done. jfb added inline comments. Comment at: clang/test/Driver/vfsmode.py:47 +# exceed PATH_MAX. +assert os.path.exists(file_relative_path) +assert not os.path.exists(os.path.join(absolute, filename)) bruno wrote: > If you want to

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

jfb marked an inline comment as done. jfb added a comment. In D65986#1623283 , @Bigcheese wrote: > This fix works, but we could also use openat to get around max path length > issues. Windows also has an API that can be used similarly. Given the type o

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

jfb marked an inline comment as done. jfb added inline comments. Comment at: clang/include/clang/Driver/Options.td:2012 + HelpText<"Use the virtual file system in 'real' mode, or 'physical' mode. In 'real' mode the working directory is linked to the process' working directory.

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

jfb updated this revision to Diff 214448. jfb marked an inline comment as done. jfb added a comment. - Rename option. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65986/new/ https://reviews.llvm.org/D65986 Files: clang/include/clang/Driver/Driv

[PATCH] D66038: [Support] heavyweight_hardware_concurrency uses affinity when counting cores fails, and never returns 0

jfb added a comment. The name of this patch is wrong? Or rather, it's 2 in 1? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66038/new/ https://reviews.llvm.org/D66038 ___ cfe-commits mailing list cfe-c

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

jfb updated this revision to Diff 214477. jfb added a comment. - Also update function name Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65986/new/ https://reviews.llvm.org/D65986 Files: clang/include/clang/Driver/Driver.h clang/include/clang/

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

jfb abandoned this revision. jfb added a comment. Let's do the better fix that Michael suggested. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65986/new/ https://reviews.llvm.org/D65986 ___ cfe-commit

[PATCH] D66046: Add new tautological compare warning for bitwise-or with a non-zero constant

jfb added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8161 + "bitwise or with non-zero value always evaluates to true">, + InGroup, DefaultIgnore; def warn_tautological_overlap_comparison : Warning< Why default ignore? CHANGES SIN

[PATCH] D66045: Improve detection of same value in comparisons

jfb added a comment. Does this work for unions as well? Comment at: test/Analysis/array-struct-region.cpp:31 + bool check() const { return this == this + 0; } + bool operator !() const { return this != this + 0; } Is this the only way? Or do casts also disa

[PATCH] D66044: Extend -Wtautological-overlap-compare to accept negative values and integer conversions

jfb added inline comments. Comment at: test/Sema/warn-unreachable.c:437 + if (~ 1) return; // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]] unaryOpNoFixit(); // expected-warning {{code will never be executed}} Why this change? CHANGES SINCE LAST ACTION htt

[PATCH] D66043: Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")

jfb added inline comments. Comment at: test/Sema/parentheses.c:156 + + (void)(x ^ b ? 1 : 2); // no warning, ^ is often used as logical xor + (void)(x || b ? 1 : 2); // no warning, logical operator I don't understand why `^` is different. What do you mean by

[PATCH] D66043: Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")

jfb added inline comments. Comment at: test/Sema/parentheses.c:156 + + (void)(x ^ b ? 1 : 2); // no warning, ^ is often used as logical xor + (void)(x || b ? 1 : 2); // no warning, logical operator rtrieu wrote: > jfb wrote: > > I don't understand why `^` is

[PATCH] D66046: Add new tautological compare warning for bitwise-or with a non-zero constant

jfb added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8161 + "bitwise or with non-zero value always evaluates to true">, + InGroup, DefaultIgnore; def warn_tautological_overlap_comparison : Warning< rtrieu wrote: > jfb wrote: > > Wh

[PATCH] D66043: Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")

jfb added inline comments. Comment at: test/Sema/parentheses.c:156 + + (void)(x ^ b ? 1 : 2); // no warning, ^ is often used as logical xor + (void)(x || b ? 1 : 2); // no warning, logical operator rtrieu wrote: > jfb wrote: > > rtrieu wrote: > > > jfb wrote:

[PATCH] D66044: Extend -Wtautological-overlap-compare to accept negative values and integer conversions

jfb added a comment. I haven't reviewed in depth, but generally this looks good. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66044/new/ https://reviews.llvm.org/D66044 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.

[PATCH] D66043: Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")

jfb accepted this revision. jfb added a comment. This revision is now accepted and ready to land. Herald added a subscriber: dexonsmith. Thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66043/new/ https://reviews.llvm.org/D66043 ___ cf

[PATCH] D66045: Improve detection of same value in comparisons

jfb added inline comments. Comment at: test/SemaCXX/self-comparison.cpp:87 +}; +} // namespace member_tests rtrieu wrote: > rtrieu wrote: > > jfb wrote: > > > The test only has `==`. Do other operators trigger? > > All the standard comparison operators will work

[PATCH] D66143: Don't use std::errc

jfb added a comment. In D66143#1627226 , @lebedev.ri wrote: > In D66143#1627221 , @ABataev wrote: > > > In D66143#1627195 , @thakis wrote: > > > > > As far as I know 4.8 is n

[PATCH] D66045: Improve detection of same value in comparisons

jfb added inline comments. Comment at: test/SemaCXX/self-comparison.cpp:87 +}; +} // namespace member_tests rtrieu wrote: > jfb wrote: > > rtrieu wrote: > > > rtrieu wrote: > > > > jfb wrote: > > > > > The test only has `==`. Do other operators trigger? > > > > A

[PATCH] D66240: Remove LVALUE / RVALUE workarounds

jfb created this revision. Herald added subscribers: llvm-commits, cfe-commits, dexonsmith, jkorous. Herald added projects: clang, LLVM. LLVM_HAS_RVALUE_REFERENCE_THIS and LLVM_LVALUE_FUNCTION shouldn't be needed anymore because the minimum compiler versions support them. Repository: rG LLVM

[PATCH] D66240: Remove LVALUE / RVALUE workarounds

jfb added a comment. BTW, if this breaks stuff maybe it's better to do it one project at a time, and remove the helper at the very end. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66240/new/ https://reviews.llvm.org/D66240 ___

[PATCH] D66240: Remove LVALUE / RVALUE workarounds

This revision was automatically updated to reflect the committed changes. Closed by commit rL368939: Remove LVALUE / RVALUE workarounds (authored by jfb, committed by ). Herald added a subscriber: kristina. Changed prior to commit: https://reviews.llvm.org/D66240?vs=215202&id=215275#toc Reposi

[PATCH] D63423: [Diagnostics] Diagnose misused xor as pow

jfb added inline comments. Comment at: include/clang/Basic/DiagnosticGroups.td:508 def Varargs : DiagGroup<"varargs">; +def XorUsedAsPow : DiagGroup<"xor-used-as-pow">; Does this match the naming that GCC ended up with? Comment at: include/c

[PATCH] D63423: [Diagnostics] Diagnose misused xor as pow

jfb added inline comments. Comment at: lib/Sema/SemaExpr.cpp:10929 +// Do not diagnose if xor keyword is used. +if (ExprStr.find("xor") != llvm::StringRef::npos) + return; xbolva00 wrote: > Quuxplusone wrote: > > xbolva00 wrote: > > > jfb wrote: > >

[PATCH] D63423: [Diagnostics] Diagnose misused xor as pow

jfb added inline comments. Comment at: lib/Sema/SemaExpr.cpp:10929 +// Do not diagnose if xor keyword is used. +if (ExprStr.find("xor") != llvm::StringRef::npos) + return; aaron.ballman wrote: > xbolva00 wrote: > > xbolva00 wrote: > > > jfb wrote: >

[PATCH] D63518: WIP BitStream reader: propagate errors

jfb added a comment. `check-clang` now passes all tests, so the patch is pretty much ready to review. I'll get started on the other parts of LLVM that use this API. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63518/new/ https://reviews.llvm.org/

[PATCH] D63518: WIP BitStream reader: propagate errors

jfb added a comment. All of `check-llvm` now passes. I'll look at other users of this stuff in the monorepo, compile and run their tests. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63518/new/ https://reviews.llvm.org/D63518 _

[PATCH] D63518: BitStream reader: propagate errors

jfb added a comment. I did a build of all LLVM monorepo projects, and only LLVM / clang / clang-tools-extra are impacted. I therefore ran tests as follows, and they all pass: rm -rf debug ; mkdir debug && (cd debug && cmake -G Ninja ../llvm -DLLVM_ENABLE_ASSERTIONS=ON -DCMAKE_BUILD_TYPE=Debu

[PATCH] D63518: BitStream reader: propagate errors

jfb marked 8 inline comments as done. jfb added inline comments. Comment at: clang-tools-extra/clang-doc/BitcodeReader.cpp:527-529 + if (llvm::Error Err = readSubBlock(BlockOrCode, I)) { +if (llvm::Error Skipped = Stream.SkipBlock()) + return Skipped; --

[PATCH] D63518: BitStream reader: propagate errors

jfb added a comment. In D63518#1558197 , @lhames wrote: > I haven't had a chance to audit the whole patch yet, but in general the error > suppression idioms are unsafe (though maybe no more so than the existing > code?). > > I would be inclined to audit

[PATCH] D63518: BitStream reader: propagate errors

jfb added inline comments. Comment at: clang-tools-extra/clang-doc/BitcodeReader.cpp:611-618 +Expected MaybeCode = Stream.ReadCode(); +if (!MaybeCode) { + // FIXME this drops the error on the floor. + consumeError(MaybeCode.takeError()); +} + +// FIX

[PATCH] D63518: BitStream reader: propagate errors

jfb added a comment. I've addressed Lang's comments and the re-audited all the new FIXME instances. This patch now actually drops errors on the floor instead of implicitly erroring out unless the error path is tested (which is what I had before). This hides bugs, but I left FIXMEs everywhere an

[PATCH] D63518: BitStream reader: propagate errors

jfb added a comment. I think this is ready to go! I rebased and ran `check-all` for LLVM / clang / clang-tools-extras and everything passes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63518/new/ https://reviews.llvm.org/D63518 __

[PATCH] D63518: BitStream reader: propagate errors

jfb marked 2 inline comments as done. jfb added a subscriber: hans. jfb added inline comments. Comment at: llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp:205 + return MaybeBitCode.takeError(); +switch (unsigned BitCode = MaybeBitCode.get()) { default: // Default be

[PATCH] D63943: ObjC: Squeeze in one more bit for the count of protocols in 'id' types

jfb added a comment. DO you have a test for that many protocols? That way when we add a diagnostic we know it won't fail. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63943/new/ https://reviews.llvm.org/D63943 ___

[PATCH] D60974: Clang IFSO driver action.

jfb added a comment. In D60974#1565054 , @plotfi wrote: > So I think I know what may be going on on your end. The llvm-readelf in your > path I believe might be the wrong lllvm-readelf (llvm-readelf-7). Are you > sure you built llvm-readelf from git? P

[PATCH] D60974: Clang IFSO driver action.

jfb added a comment. In D60974#1565517 , @jfb wrote: > In D60974#1565054 , @plotfi wrote: > > > So I think I know what may be going on on your end. The llvm-readelf in > > your path I believe might be the wrong lll

[PATCH] D60974: Clang IFSO driver action.

jfb added a comment. In D60974#1565541 , @plotfi wrote: > In D60974#1565527 , @jfb wrote: > > > In D60974#1565517 , @jfb wrote: > > > > > In D60974#1565054

[PATCH] D60974: Clang IFSO driver action.

jfb added a comment. Looking at the code quickly, I'm not sure that this should be in clang itself. It sounds like a better fit for a clang-based tool, and not clang. Why does it need to be part of clang? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.o

[PATCH] D60974: Clang IFSO driver action.

jfb added subscribers: arphaman, ributzka, Bigcheese. jfb added a comment. In D60974#1565621 , @plotfi wrote: > In D60974#1565577 , @jfb wrote: > > > Looking at the code quickly, I'm not sure that this should be in

[PATCH] D59254: [RFC] Implementation of Clang randstruct

jfb added a comment. Do you intend on moving this forward? It seems like a Good Thing overall, and I'd love to help review some more. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59254/new/ https://reviews.llvm.org/D59254 _

[PATCH] D59254: [RFC] Implementation of Clang randstruct

jfb added a comment. In D59254#1566711 , @shawnl wrote: > I think the essential functionality of this patch should be in LLVM and not > Clang, so that all front-ends can benefit. Too many generally useful things > are in Clang when they should be in LLVM

[PATCH] D59254: [RFC] Implementation of Clang randstruct

jfb added a comment. In D59254#1566895 , @shawnl wrote: > My point is that most languages these days that intend to be compiled to > machine code want compatibility with the C ABI, and randstruct will be part > of that (and can be made compatible between

[PATCH] D64128: [CodeGen] Generate llvm.ptrmask instead of inttoptr(and(ptrtoint, C)) if possible.

jfb added a comment. I'm not sure I understand all the implications, and why that would / wouldn't be valid. Should this be an builtin that can be called from C++ directly? Comment at: clang/lib/CodeGen/CGExprScalar.cpp:2034 + (AllOnes << llvm::Log2_64(DL.getPointerABIA

[PATCH] D64164: [clang][ARM] Fix msvc arm{64} builtins to use int on LLP64 systems.

jfb accepted this revision. jfb added inline comments. This revision is now accepted and ready to land. Comment at: test/CodeGen/ms-intrinsics-other.c:212 + return _InterlockedAdd(Addend, Value); +} + Looks like only `cmpxchg` preserves `volatile`? That's not yo

[PATCH] D63518: BitStream reader: propagate errors

jfb marked 2 inline comments as done. jfb added inline comments. Comment at: clang-tools-extra/trunk/clang-doc/BitcodeReader.cpp:619 +// FIXME check that the enum is in range. +auto Code = static_cast(MaybeCode.get()); + bjope wrote: > This has caused pro

[PATCH] D63518: BitStream reader: propagate errors

jfb marked an inline comment as done. jfb added inline comments. Comment at: clang-tools-extra/trunk/clang-doc/BitcodeReader.cpp:619 +// FIXME check that the enum is in range. +auto Code = static_cast(MaybeCode.get()); + jfb wrote: > bjope wrote: > > This

[PATCH] D64262: Bitstream reader: Fix undefined behavior seen after rL364464

jfb accepted this revision. jfb added a comment. This revision is now accepted and ready to land. Thanks for catching this! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64262/new/ https://reviews.llvm.org/D64262

[PATCH] D64382: Use getMostFrequentByte to decide if should used memset+stores

jfb added inline comments. Comment at: clang/lib/CodeGen/CGDecl.cpp:865 +/// initializer with equal or fewer than NumStores scalar stores. +static bool canEmitStoresForInitAfterMemset(CodeGenModule &CGM, +llvm::Constant *Init, -

[PATCH] D60593: [GwpAsan] Introduce GWP-ASan.

jfb added a comment. In D60593#1498622 , @hctim wrote: > In D60593#1495428 , @gribozavr wrote: > > > What does GWP stand for? > > > Google Wide Profiling >

[PATCH] D61923: [GWP-ASan] Mutex implementation [2].

jfb added a comment. In D61923#1502323 , @hctim wrote: > In D61923#1502245 , @jfb wrote: > > > Seems a shame to duplicate mutex again... Why can't use use the STL's > > version again? It doesn't allocate. > > > We

[PATCH] D61923: [GWP-ASan] Mutex implementation [2].

jfb added a comment. > In D61923#1502404 , @jfb wrote: > >> > We can't use `std::mutex` as we must be C-compatible >> >> Can you clarify what you mean by this? I don't understand. >> Have you asked on libcxx-dev? Is there interest in the libc++ community

[PATCH] D54604: Automatic variable initialization

jfb updated this revision to Diff 178129. jfb marked 3 inline comments as done. jfb added a comment. - Don't document 'zero'. - Make drv_trivial_auto_var_init_zero_disabled an error instead of a warning. - Change the parameter: we only have [[clang::uninitialized]] now. Repository: rC Clang C

[PATCH] D54604: Automatic variable initialization

jfb added a comment. In D54604#1327893 , @rsmith wrote: > For the record: I'm OK with this direction. (I somewhat prefer removing the > `-enable-long-wordy-thing` option and instead automatically disabling the > `zero` option for clang release builds, bu

[PATCH] D54604: Automatic variable initialization

jfb added a comment. In D54604#1331711 , @pcc wrote: > Mostly looking good, a few style nits. Thanks! I think I addressed all of your comments. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54604/new/ https://reviews.ll

[PATCH] D54604: Automatic variable initialization

jfb updated this revision to Diff 178301. jfb marked 6 inline comments as done. jfb added a comment. - Address @pcc's comments. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54604/new/ https://reviews.llvm.org/D54604 Files: include/clang/Basic/Attr.td includ

[PATCH] D54604: Automatic variable initialization

jfb updated this revision to Diff 178304. jfb added a comment. - Rebase Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54604/new/ https://reviews.llvm.org/D54604 Files: include/clang/Basic/Attr.td include/clang/Basic/AttrDocs.td include/clang/Basic/Diagnost

[PATCH] D54604: Automatic variable initialization

jfb updated this revision to Diff 178566. jfb added a comment. - Update attribute test. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54604/new/ https://reviews.llvm.org/D54604 Files: include/clang/Basic/Attr.td include/clang/Basic/AttrDocs.td include/clan

[PATCH] D54604: Automatic variable initialization

This revision was automatically updated to reflect the committed changes. Closed by commit rL349442: Automatic variable initialization (authored by jfb, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D54604?vs=178566&id=178590#toc Rep

[PATCH] D55895: NFC: simplify Darwin environment handling

jfb created this revision. jfb added reviewers: dexonsmith, arphaman. Herald added subscribers: cfe-commits, jkorous. The previous code detected conflicts through copy-pasta, this versions uses a 'loop'. Repository: rC Clang https://reviews.llvm.org/D55895 Files: lib/Driver/ToolChains/Darw

[PATCH] D67399: [ARM] Follow AACPS standard for volatile bitfields

jfb added inline comments. Comment at: clang/test/CodeGen/aapcs-bitfield.c:541 // BE-NEXT:[[TMP0:%.*]] = getelementptr inbounds [[STRUCT_ST9:%.*]], %struct.st9* [[M:%.*]], i32 0, i32 0 +// BE-NEXT:[[BF_LOAD:%.*]] = load volatile i8, i8* [[TMP0]], align 4 // BE-NEXT:

[PATCH] D64146: [Clang Interpreter] Initial patch for the constexpr interpreter

jfb accepted this revision. jfb added a comment. Sounds like this is ready to land again! Thanks for fixing everything. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64146/new/ https://reviews.llvm.org/D64146

[PATCH] D67399: [ARM] Follow AACPS standard for volatile bitfields

jfb added a comment. In D67399#1669038 , @dnsampaio wrote: > Indeed our main concern is regarding the access widths of loads. As mentioned > by @rjmccall, most volatile bitfields are used to perform memory mapped I/O, > and some hardware only support the

[PATCH] D67436: CodeGen: set correct result for atomic compound expressions

jfb accepted this revision. jfb added a comment. This revision is now accepted and ready to land. Herald added a subscriber: dexonsmith. Separately, does this do floating-point add / sub properly? We added them too C++20. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https:

[PATCH] D68115: Zero initialize padding in unions

jfb added a comment. The entire point of this feature is to add guardrails to the language. What do people expect in the real world? Is there a cost to meeting these expectations? If we put the pattern (0x00 or 0xaa) in the technically undef space, what comes out? Repository: rG LLVM Github

[PATCH] D46024: [clang-format] Add SpaceBeforeCpp11BracedList option.

jfb accepted this revision. jfb added a comment. This revision is now accepted and ready to land. On the WebKit side this lgtm. Let's leave some time for clang-format folks to chime in. Repository: rC Clang https://reviews.llvm.org/D46024 ___ cf

[PATCH] D46386: Adding __atomic_fetch_min/max intrinsics to clang

jfb added inline comments. Comment at: lib/Sema/SemaChecking.cpp:3098 + case AtomicExpr::AO__atomic_fetch_umax: +IsMinMax = true; +Form = Arithmetic; Should `__sync_fetch_and_min` and others also set `IsMinMax`? Repository: rC Clang https://reviews.

[PATCH] D45470: Emit an error when mixing and

jfb added a comment. This isn't bad, so I'd go with it, but separately I imagine that we could implement the suggestion in http://wg21.link/p0943 and expose it even before C++20? Not sure we do this much, but I'd argue that before that fix stdatomic.h is just useless anyways, so it's a fine bre

[PATCH] D42933: [Sema] Avoid -Wformat warning for NSInteger/NSUInteger 'int' values with %zu/%zi long specifiers

jfb added a comment. Herald added a reviewer: javed.absar. I was just looking at this, and I think @arphaman's patch is pretty much the right approach (with 2 suggested fixes below). I don't think the code we're currently warning on is broken: a user code has `NSInteger` with `%zd` or `NSUInteg

[PATCH] D42933: [Sema] Avoid -Wformat warning for NSInteger/NSUInteger 'int' values with %zu/%zi long specifiers

jfb added a comment. In https://reviews.llvm.org/D42933#1090286, @smeenai wrote: > Note that the alignment matters in addition to the size. Sure, but AFAICT from `./lib/Basic/Targets/*` the alignment is also specified properly, is it not? > The pattern I've seen internally is people using `%z

[PATCH] D23041: Un-XFAIL GCC atomics.align

jfb added a comment. Herald added subscribers: christof, aheejin. In https://reviews.llvm.org/D23041#632708, @EricWF wrote: > Have you filed a bug against GCC regarding its current behavior? > > Also it seems like a bad idea to add `-fabi-version=6`, since it selects an > older ABI version and n

[PATCH] D22711: Diagnose invalid failure memory orderings.

jfb requested changes to this revision. jfb added a comment. This revision now requires changes to proceed. Given http://wg21.link/p0418 I think this requires an update. I don't think the old behavior is worth supporting in older `-std=` versions, unless we're worried that it would make code les

[PATCH] D48852: [Sema] -Wformat-pedantic only for NSInteger/NSUInteger %tu/%td on Darwin

jfb accepted this revision. jfb added a comment. This revision is now accepted and ready to land. LGTM after a few questions. Comment at: include/clang/Analysis/Analyses/FormatString.h:265 + enum class TypeKind { Unspecified, SizeT, PtrdiffT }; + TypeKind TK = TypeKind::Unspe

[PATCH] D49209: CodeGen: specify alignment for automatic variable initialization

jfb created this revision. Herald added subscribers: cfe-commits, dexonsmith. Automatic variable initialization was generating default-aligned stores (which are deprecated) instead of using the known alignment from the alloca. Repository: rC Clang https://reviews.llvm.org/D49209 Files: li

[PATCH] D49209: CodeGen: specify alignment for automatic variable initialization

jfb updated this revision to Diff 155285. jfb marked an inline comment as done. jfb added a comment. - Use Address as suggested in review. Repository: rC Clang https://reviews.llvm.org/D49209 Files: lib/CodeGen/CGBuilder.h lib/CodeGen/CGDecl.cpp test/CodeGen/init.c test/CodeGenOpenCL

[PATCH] D49209: CodeGen: specify alignment for automatic variable initialization

jfb added a comment. I updated the patch to use `Address`, and also use `inbounds`. This was a bit tricky because of the GEPs. I also updated tests which run into this codegen. Only the following tests actually hit this code, and not all check these stores: Clang :: CodeGen/2004-03-09-LargeAr

[PATCH] D49209: CodeGen: specify alignment for automatic variable initialization

jfb updated this revision to Diff 155287. jfb added a comment. - Fix silly naming and lookup. Repository: rC Clang https://reviews.llvm.org/D49209 Files: lib/CodeGen/CGBuilder.h lib/CodeGen/CGDecl.cpp test/CodeGen/init.c test/CodeGenOpenCL/partial_initializer.cl Index: test/CodeGenO

[PATCH] D49209: CodeGen: specify alignment for automatic variable initialization

jfb updated this revision to Diff 155424. jfb added a comment. - Simplify CreateStore. Repository: rC Clang https://reviews.llvm.org/D49209 Files: lib/CodeGen/CGBuilder.h lib/CodeGen/CGDecl.cpp test/CodeGen/init.c test/CodeGenOpenCL/partial_initializer.cl Index: test/CodeGenOpenCL/p

[PATCH] D49209: CodeGen: specify alignment for automatic variable initialization

jfb marked an inline comment as done. jfb added inline comments. Comment at: lib/CodeGen/CGBuilder.h:260 +CharUnits::fromQuantity(Offset.getSExtValue(; + } + efriedma wrote: > Not sure about the new helper. We already have Create

[PATCH] D49209: CodeGen: specify alignment for automatic variable initialization

This revision was automatically updated to reflect the committed changes. Closed by commit rL337041: CodeGen: specify alignment + inbounds for automatic variable initialization (authored by jfb, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.o

[PATCH] D49458: Support implicit _Atomic struct load / store

jfb created this revision. jfb added a reviewer: dexonsmith. Herald added a subscriber: cfe-commits. Using _Atomic to do implicit load / store is just a seq_cst atomic_load / atomic_store. Stores currently assert in Sema::ImpCastExprToType with 'can't implicitly cast lvalue to rvalue with this c

[PATCH] D46112: Allow _Atomic to be specified on incomplete types

jfb added a comment. In https://reviews.llvm.org/D46112#1113557, @aaron.ballman wrote: > In https://reviews.llvm.org/D46112#1098243, @rsmith wrote: > > > In https://reviews.llvm.org/D46112#1096893, @aaron.ballman wrote: > > > > > In https://reviews.llvm.org/D46112#1091981, @efriedma wrote: > > >

[PATCH] D49458: Support implicit _Atomic struct load / store

jfb added a comment. I'm sure there are other bugs around `_Atomic`, please file a bug an CC me if that's the case. I'll commit this fix for now. Repository: rC Clang https://reviews.llvm.org/D49458 ___ cfe-commits mailing list cfe-commits@lists

[PATCH] D49458: Support implicit _Atomic struct load / store

This revision was automatically updated to reflect the committed changes. Closed by commit rL337410: Support implicit _Atomic struct load / store (authored by jfb, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.org/D49458 Files: cfe/trunk/l

[PATCH] D66046: Add new tautological compare warning for bitwise-or with a non-zero constant

jfb added a comment. In D66046#1717229 , @thakis wrote: > Mr Trieu, what do you think about adding some or all of the > Wtautological-compare warnings to Wall It's addressed in the patch description: https://reviews.llvm.org/D66046?id=214484#inline-591

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

jfb added a comment. In D71726#2047566 , @ldionne wrote: > In D71726#1791904 , @jfb wrote: > > > This generally seems fine. Does it work on most backends? I want to make > > sure it doesn't fail in backends :) > >

[PATCH] D78938: Fixing all comparisons for C++20 compilation.

jfb accepted this revision. jfb added a comment. One suggestions, otherwise looks good. Thanks for doing this :) Comment at: llvm/include/llvm/ADT/DirectedGraph.h:97 + } + friend bool operator!=(const NodeType &M, const NodeType &N) { !(M == N); } davidston

[PATCH] D80055: Diagnose union tail padding

jfb added a comment. In D80055#2042630 , @rsmith wrote: > I'm not convinced that this is an especially useful diagnostic (much like > with `-Wpadded`), but I'm also not opposed if you are aware of cases where it > would be used. I wrote it to help with

<    1   2   3   4   5   6   >