[PATCH] D54604: Automatic variable initialization

2018-12-07 Thread JF Bastien via Phabricator via cfe-commits
jfb added a comment. I added an option that's required when using `clang` directly (*not* `-cc1`!) If you use `-ftrivial-auto-var-init=zero` without `-enable-trivial-auto-var-init-zero-knowning-it-will-be-removed-from-clang` in `clang` it'll generate a warning and change initialization to

[PATCH] D54604: Automatic variable initialization

2018-12-07 Thread JF Bastien via Phabricator via cfe-commits
jfb updated this revision to Diff 177328. jfb added a comment. - Add an ugly option to enable zero init Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54604/new/ https://reviews.llvm.org/D54604 Files: include/clang/Basic/Attr.td

[PATCH] D54604: Automatic variable initialization

2018-12-07 Thread JF Bastien via Phabricator via cfe-commits
jfb added a comment. I've addressed @pcc's comments. I'll now update the patch to mandate a `-Xclang` option for zero-init (otherwise it'll warn and pattern-init instead), and remove documentation for zero-init. I'm also thinking of changing float init to negative NaN with infinite scream

[PATCH] D54604: Automatic variable initialization

2018-12-07 Thread JF Bastien via Phabricator via cfe-commits
jfb updated this revision to Diff 177262. jfb marked 10 inline comments as done. jfb added a comment. - Make sure uninit-variables.c doesn't break. - Address Peter's comments, improve tests. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54604/new/

[PATCH] D54604: Automatic variable initialization

2018-11-16 Thread JF Bastien via Phabricator via cfe-commits
jfb added a comment. In https://reviews.llvm.org/D54604#1301807, @kcc wrote: > Would it be possible to extend test/Sema/uninit-variables.c to verify that > the new option > doesn't break -Wuninitialized? Done. Repository: rC Clang https://reviews.llvm.org/D54604

[PATCH] D54604: Automatic variable initialization

2018-11-16 Thread JF Bastien via Phabricator via cfe-commits
jfb updated this revision to Diff 174471. jfb added a comment. - Make sure uninit-variables.c doesn't break. Repository: rC Clang https://reviews.llvm.org/D54604 Files: include/clang/Basic/Attr.td include/clang/Basic/AttrDocs.td include/clang/Basic/LangOptions.def

[PATCH] D54604: Automatic variable initialization

2018-11-15 Thread JF Bastien via Phabricator via cfe-commits
jfb added a comment. In https://reviews.llvm.org/D54604#1300544, @t.p.northover wrote: > > This isn't meant to change the semantics of C and C++. > > As I said in the cfe-dev thread, that's exactly what it does. Specifically I > think this is essentially defining a new dialect of C++, which I

[PATCH] D54604: Automatic variable initialization

2018-11-15 Thread JF Bastien via Phabricator via cfe-commits
jfb created this revision. jfb added reviewers: kcc, rjmccall, rsmith. Herald added subscribers: dexonsmith, jkorous, JDevlieghere. Add an option to initialize automatic variables with either a pattern or with zeroes. The default is still that automatic variables are uninitialized. Also add

[PATCH] D54055: CGDecl::emitStoresForConstant fix synthesized constant's name

2018-11-14 Thread JF Bastien via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. jfb marked an inline comment as done. Closed by commit rL346915: CGDecl::emitStoresForConstant fix synthesized constants name (authored by jfb, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM

[PATCH] D54055: CGDecl::emitStoresForConstant fix synthesized constant's name

2018-11-14 Thread JF Bastien via Phabricator via cfe-commits
jfb marked an inline comment as done. jfb added inline comments. Comment at: lib/CodeGen/CGDecl.cpp:990-998 + std::string Name = ("__const." + FunctionName(D.getParentFunctionOrMethod()) + + "." + D.getName()) + .str(); +

[PATCH] D54055: CGDecl::emitStoresForConstant fix synthesized constant's name

2018-11-14 Thread JF Bastien via Phabricator via cfe-commits
jfb updated this revision to Diff 174090. jfb added a comment. - Inline the name creating, save a heap allocation. Repository: rC Clang https://reviews.llvm.org/D54055 Files: lib/CodeGen/CGDecl.cpp test/CodeGen/decl.c test/CodeGen/dump-struct-builtin.c

[PATCH] D54055: CGDecl::emitStoresForConstant fix synthesized constant's name

2018-11-14 Thread JF Bastien via Phabricator via cfe-commits
jfb added a comment. Small update to use `std::string` because `Twine` lifetimes are sad. This didn't trigger on any of the tests, but did in a stage2 build. Repository: rC Clang https://reviews.llvm.org/D54055 ___ cfe-commits mailing list

[PATCH] D54055: CGDecl::emitStoresForConstant fix synthesized constant's name

2018-11-14 Thread JF Bastien via Phabricator via cfe-commits
jfb updated this revision to Diff 174087. jfb added a comment. Herald added a subscriber: jkorous. - Fix lifetime of the name, Twine bites again. Repository: rC Clang https://reviews.llvm.org/D54055 Files: lib/CodeGen/CGDecl.cpp test/CodeGen/decl.c test/CodeGen/dump-struct-builtin.c

[PATCH] D54055: CXXNameMangler::mangleFunctionParam: fix assertion

2018-11-13 Thread JF Bastien via Phabricator via cfe-commits
jfb updated this revision to Diff 173946. jfb added a comment. - clang-format Repository: rC Clang https://reviews.llvm.org/D54055 Files: lib/CodeGen/CGDecl.cpp test/CodeGen/decl.c test/CodeGen/dump-struct-builtin.c test/CodeGenCXX/amdgcn-string-literal.cpp

[PATCH] D54055: CXXNameMangler::mangleFunctionParam: fix assertion

2018-11-13 Thread JF Bastien via Phabricator via cfe-commits
jfb added a comment. In https://reviews.llvm.org/D54055#1286523, @rjmccall wrote: > Pfft, if I'm going to be held responsible for my own work, I'm in a lot of > trouble. :) > > Function name + local variable name WFM. Your wish is my command (in this specific instance anyways). Repository:

[PATCH] D54055: CXXNameMangler::mangleFunctionParam: fix assertion

2018-11-13 Thread JF Bastien via Phabricator via cfe-commits
jfb updated this revision to Diff 173945. jfb added a comment. Herald added subscribers: nhaehnle, jvesely. - As discussed on the review, change the constant generation to name the synthesized constant using '__const.' + function_name + '.' variable_name. The previous code wasn't generally

[PATCH] D54055: CXXNameMangler::mangleFunctionParam: fix assertion

2018-11-03 Thread JF Bastien via Phabricator via cfe-commits
jfb added a comment. In https://reviews.llvm.org/D54055#1286514, @rjmccall wrote: > In https://reviews.llvm.org/D54055#1286397, @jfb wrote: > > > In https://reviews.llvm.org/D54055#1286396, @rjmccall wrote: > > > > > That sounds more like this use of the mangler isn't manipulating the > > >

[PATCH] D54055: CXXNameMangler::mangleFunctionParam: fix assertion

2018-11-02 Thread JF Bastien via Phabricator via cfe-commits
jfb added a comment. In https://reviews.llvm.org/D54055#1286396, @rjmccall wrote: > That sounds more like this use of the mangler isn't manipulating the function > type context correctly. But actually I think the problem is that it's > ridiculous to assume that arbitrary local declarations

[PATCH] D54055: CXXNameMangler::mangleFunctionParam: fix assertion

2018-11-02 Thread JF Bastien via Phabricator via cfe-commits
jfb added a comment. In https://reviews.llvm.org/D54055#1286196, @erik.pilkington wrote: > Can you give an example of some code that triggered this with your patch > applied? Even if it isn't a real reproducer right now, it would help to try > to understand whats happening here. Sure! I

[PATCH] D54055: CXXNameMangler::mangleFunctionParam: fix assertion

2018-11-02 Thread JF Bastien via Phabricator via cfe-commits
jfb added a comment. I'm not sure this is the right fix because mangling confuses me. It fixes the assertion I'm encountering in my patch, and I don't think I can create a repro without the patch (since I'm asking to mangle a local in a way we don't seem to right now). Repository: rC Clang

[PATCH] D54055: CXXNameMangler::mangleFunctionParam: fix assertion

2018-11-02 Thread JF Bastien via Phabricator via cfe-commits
jfb created this revision. jfb added a reviewer: rjmccall. Herald added subscribers: cfe-commits, dexonsmith. jfb added a comment. I'm not sure this is the right fix because mangling confuses me. It fixes the assertion I'm encountering in my patch, and I don't think I can create a repro without

[PATCH] D53154: [CodeGen] Handle extern references to OBJC_CLASS_$_*

2018-10-11 Thread JF Bastien via Phabricator via cfe-commits
jfb added a comment. Overall this seems fine, but I'm no ObjC expert either so I'll defer to @rjmccall Comment at: clang/lib/CodeGen/CGObjCMac.cpp:7193 + if (!GV || GV->getType() != ObjCTypes.ClassnfABITy->getPointerTo()) { +auto *NewGV = new

[PATCH] D52949: [Diagnostics] Implement -Wsizeof-pointer-div

2018-10-09 Thread JF Bastien via Phabricator via cfe-commits
jfb added a comment. Can you add tests with arrays? The error message doesn't really say what to do instead. What's wrong? What's correct instead? In C++17 and later we should suggest using `std::size` for some cases instead. https://reviews.llvm.org/D52949

[PATCH] D53001: [Driver] check for exit code from SIGPIPE

2018-10-08 Thread JF Bastien via Phabricator via cfe-commits
jfb accepted this revision. jfb added a comment. This revision is now accepted and ready to land. Digging through `tools/driver/driver.cpp` this seems to be the right thing. Maybe leave it open for a bit so others can chime in (if say they have other drivers or whatever?). Thanks for fixing!

[PATCH] D53001: [Driver] check for exit code from SIGPIPE

2018-10-08 Thread JF Bastien via Phabricator via cfe-commits
jfb added a comment. Just to be sure: what's the exit code from the driver? If we don't diagnose I'm fine with it... but the exit code still needs to reflect the failure! Repository: rC Clang https://reviews.llvm.org/D53001 ___ cfe-commits

[PATCH] D53001: [Driver] check for exit code from SIGPIPE

2018-10-08 Thread JF Bastien via Phabricator via cfe-commits
jfb added inline comments. Comment at: lib/Driver/Driver.cpp:1406 +// for SIGPIPE. Do not print diagnostics for this case. +if (Res == 71) + continue; nickdesaulniers wrote: > jfb wrote: > > jfb wrote: > > > Ditto on magical number in a header. > >

[PATCH] D53001: [Driver] check for exit code from SIGPIPE

2018-10-08 Thread JF Bastien via Phabricator via cfe-commits
jfb added a comment. What's the return code of the driver when the pipe is broken that way? Comment at: lib/Driver/Driver.cpp:1406 +// for SIGPIPE. Do not print diagnostics for this case. +if (Res == 71) + continue; Ditto on magical number in a

[PATCH] D52892: [Clang-tidy] readability check to convert numerical constants to std::numeric_limits

2018-10-04 Thread JF Bastien via Phabricator via cfe-commits
jfb added a comment. Can you also catch casts such as `(unsigned)-1` ? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52892 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

2018-09-21 Thread JF Bastien via Phabricator via cfe-commits
jfb added a comment. In https://reviews.llvm.org/D52339#1242103, @aaron.ballman wrote: > In https://reviews.llvm.org/D52339#1242086, @jfb wrote: > > > I think we should consider proposing this to the C committee. > > @aaron.ballman I can help Erik write the paper, would you be able to > >

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

2018-09-21 Thread JF Bastien via Phabricator via cfe-commits
jfb added a comment. I think we should consider proposing this to the C committee. @aaron.ballman I can help Erik write the paper, would you be able to present it? Too tight for the upcoming meeting, but I'm guessing we have *plenty* of time for ++C17. Repository: rC Clang

[PATCH] D51752: NFC: deduplicate isRepeatedBytePattern from clang to LLVM's isBytewiseValue

2018-09-21 Thread JF Bastien via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rL342734: NFC: deduplicate isRepeatedBytePattern from clang to LLVMs isBytewiseValue (authored by jfb, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM

[PATCH] D51084: Implement -Watomic-implicit-seq-cst

2018-09-10 Thread JF Bastien via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rC341860: Implement -Watomic-implicit-seq-cst (authored by jfb, committed by ). Changed prior to commit: https://reviews.llvm.org/D51084?vs=164235=164746#toc Repository: rC Clang

[PATCH] D51752: NFC: deduplicate isRepeatedBytePattern from clang to LLVM's isBytewiseValue

2018-09-07 Thread JF Bastien via Phabricator via cfe-commits
jfb added a comment. Will wait on https://reviews.llvm.org/D51751 before committing, otherwise clang tests will start failing (because current `isRepeatedBytePattern` isn't capable enough). Repository: rC Clang https://reviews.llvm.org/D51752

[PATCH] D51752: NFC: move isRepeatedBytePattern from clang to Constant

2018-09-07 Thread JF Bastien via Phabricator via cfe-commits
jfb updated this revision to Diff 164481. jfb added a comment. - Use isBytewiseValue instead. Repository: rC Clang https://reviews.llvm.org/D51752 Files: lib/CodeGen/CGDecl.cpp Index: lib/CodeGen/CGDecl.cpp === ---

[PATCH] D51752: NFC: move isRepeatedBytePattern from clang to Constant

2018-09-06 Thread JF Bastien via Phabricator via cfe-commits
jfb added a comment. In https://reviews.llvm.org/D51752#1226474, @MatzeB wrote: > In https://reviews.llvm.org/D51752#1226462, @MatzeB wrote: > > > - I assume the review lacks the changes on the llvm side? > > > Oops missed the link to the llvm changes in the description... At least in > theory

[PATCH] D51752: NFC: move isRepeatedBytePattern from clang to Constant

2018-09-06 Thread JF Bastien via Phabricator via cfe-commits
jfb added a comment. In https://reviews.llvm.org/D51752#1226465, @jfb wrote: > In https://reviews.llvm.org/D51752#1226462, @MatzeB wrote: > > > - I'm not a good person to review clang changes. > > - I assume the review lacks the changes on the llvm side? > > - On the LLVM side this feels like

[PATCH] D51752: NFC: move isRepeatedBytePattern from clang to Constant

2018-09-06 Thread JF Bastien via Phabricator via cfe-commits
jfb added a comment. In https://reviews.llvm.org/D51752#1226462, @MatzeB wrote: > - I'm not a good person to review clang changes. > - I assume the review lacks the changes on the llvm side? > - On the LLVM side this feels like something for `Analysis/ValueTracking.h` > in fact I already see

[PATCH] D51752: NFC: move isRepeatedBytePattern from clang to Constant

2018-09-06 Thread JF Bastien via Phabricator via cfe-commits
jfb created this revision. Herald added subscribers: cfe-commits, dexonsmith. This code was in CGDecl.cpp and really belongs to Constant. This will allow me to use it in a subsequent patch. LLVM part of this patch: https://reviews.llvm.org/D51751 Repository: rC Clang

[PATCH] D51084: Implement -Watomic-implicit-seq-cst

2018-09-06 Thread JF Bastien via Phabricator via cfe-commits
jfb updated this revision to Diff 164235. jfb marked 4 inline comments as done. jfb added a comment. - Address comments. Repository: rC Clang https://reviews.llvm.org/D51084 Files: include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaChecking.cpp test/Sema/atomic-implicit-seq_cst.c

[PATCH] D51084: Implement -Watomic-implicit-seq-cst

2018-09-06 Thread JF Bastien via Phabricator via cfe-commits
jfb added inline comments. Comment at: lib/Sema/SemaChecking.cpp:10974 + if (E->IgnoreParenImpCasts()->getType()->isAtomicType()) +return; CheckImplicitConversion(S, E->IgnoreParenImpCasts(), S.Context.BoolTy, CC); rjmccall wrote: > Can you explain this

[PATCH] D51084: Implement -Watomic-implicit-seq-cst

2018-08-31 Thread JF Bastien via Phabricator via cfe-commits
jfb updated this revision to Diff 163577. jfb added a comment. - Don't diagnose initialization, only assignment. Repository: rC Clang https://reviews.llvm.org/D51084 Files: include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaChecking.cpp test/Sema/atomic-implicit-seq_cst.c

[PATCH] D51084: Implement -Watomic-implicit-seq-cst

2018-08-29 Thread JF Bastien via Phabricator via cfe-commits
jfb added a comment. In https://reviews.llvm.org/D51084#1216913, @rjmccall wrote: > It says the type of the assignment expression, not the type of the LHS. > > C11 [6.5.16]p2: "The type of an assignment expression is the type the left > operand would have after lvalue conversion." > > C11

[PATCH] D51084: Implement -Watomic-implicit-seq-cst

2018-08-28 Thread JF Bastien via Phabricator via cfe-commits
jfb added inline comments. Comment at: lib/Sema/SemaChecking.cpp:10668 + if (Source->isAtomicType() || Target->isAtomicType()) +S.Diag(E->getBeginLoc(), diag::warn_atomic_implicit_seq_cst); + rjmccall wrote: > jfb wrote: > > rjmccall wrote: > > > Why would

[PATCH] D51084: Implement -Watomic-implicit-seq-cst

2018-08-28 Thread JF Bastien via Phabricator via cfe-commits
jfb added inline comments. Comment at: lib/Sema/SemaChecking.cpp:10668 + if (Source->isAtomicType() || Target->isAtomicType()) +S.Diag(E->getBeginLoc(), diag::warn_atomic_implicit_seq_cst); + rjmccall wrote: > Why would the target be an atomic type? And if

[PATCH] D51314: Parse compile commands lazily in InterpolatingCompilationDatabase

2018-08-27 Thread JF Bastien via Phabricator via cfe-commits
jfb added inline comments. Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:124 +// A CompileCommand that can be applied to another file. Any instance of this +// object is invalid after std::move() from it. struct TransferableCommand { This comment

[PATCH] D51314: Parse compile commands lazily in InterpolatingCompilationDatabase

2018-08-27 Thread JF Bastien via Phabricator via cfe-commits
jfb requested changes to this revision. jfb added inline comments. This revision now requires changes to proceed. Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:201 + + CommandTraits computeTraits() const { +CommandTraits Result; It's not

[PATCH] D51084: Implement -Watomic-implicit-seq-cst

2018-08-24 Thread JF Bastien via Phabricator via cfe-commits
jfb added a comment. Updated. Repository: rC Clang https://reviews.llvm.org/D51084 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51084: Implement -Watomic-implicit-seq-cst

2018-08-24 Thread JF Bastien via Phabricator via cfe-commits
jfb updated this revision to Diff 162484. jfb marked 2 inline comments as done. jfb added a comment. - Address John's comments: diagnose at beginning, and don't check isIgnored manually. Repository: rC Clang https://reviews.llvm.org/D51084 Files:

[PATCH] D51170: [libc++] Remove race condition in std::async

2018-08-23 Thread JF Bastien via Phabricator via cfe-commits
jfb accepted this revision. jfb added inline comments. This revision is now accepted and ready to land. Comment at: libcxx/include/future:556 bool __has_value() const {return (__state_ & __constructed) || (__exception_ != nullptr);} I'm not

[PATCH] D51084: Implement -Watomic-implicit-seq-cst

2018-08-21 Thread JF Bastien via Phabricator via cfe-commits
jfb created this revision. jfb added a reviewer: rjmccall. Herald added subscribers: cfe-commits, dexonsmith. _Atomic and __sync_* operations are implicitly sequentially-consistent. Some codebases want to force explicit usage of memory order instead. This warning allows them to know where

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

2018-08-21 Thread JF Bastien via Phabricator via cfe-commits
jfb added a comment. Isn't this duplicating code in lib/Support/Unix/Threading.inc with a different implementation? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50993 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

2018-08-20 Thread JF Bastien via Phabricator via cfe-commits
jfb accepted this revision. jfb added inline comments. Comment at: clang/include/clang/AST/Decl.h:1472 + /// Do we need to emit an exit-time destructor for this variable? + bool isNoDestroy(const ASTContext &) const; erik.pilkington wrote: > jfb wrote: > >

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

2018-08-20 Thread JF Bastien via Phabricator via cfe-commits
jfb added inline comments. Comment at: clang/include/clang/AST/Decl.h:1472 + /// Do we need to emit an exit-time destructor for this variable? + bool isNoDestroy(const ASTContext &) const; rsmith wrote: > jfb wrote: > > This is only valid for static

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

2018-08-20 Thread JF Bastien via Phabricator via cfe-commits
jfb added inline comments. Comment at: clang/include/clang/AST/Decl.h:1472 + /// Do we need to emit an exit-time destructor for this variable? + bool isNoDestroy(const ASTContext &) const; This is only valid for static variables, right? It's probably better

[PATCH] D50719: [libc++] Fix incorrect definition of TEST_HAS_C11_FEATURES

2018-08-14 Thread JF Bastien via Phabricator via cfe-commits
jfb accepted this revision. jfb added a comment. This revision is now accepted and ready to land. From `__ISO_C_VISIBLE >= 2011` it looks like this tries to test C11 features regardless of the C++ version. That's probably fine, but we're walking this line where only C++17 really guarantees C11

[PATCH] D50549: [libcxx] [test] Repair thread unsafety in thread tests

2018-08-13 Thread JF Bastien via Phabricator via cfe-commits
jfb added a comment. CC some sanitizer folks. https://reviews.llvm.org/D50549 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44671: [libcxx] Enable static libcxxabi linking on Darwin

2018-08-08 Thread JF Bastien via Phabricator via cfe-commits
jfb added a comment. Were the concerns expressed in https://reviews.llvm.org/D8017 addressed? Repository: rCXX libc++ https://reviews.llvm.org/D44671 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D45639: [Driver] Support default libc++ library location on Darwin

2018-08-08 Thread JF Bastien via Phabricator via cfe-commits
jfb added a comment. In https://reviews.llvm.org/D45639#1192849, @beanz wrote: > Adding @jfb since this is his domain now too. @ldionne is the libc++ expert :-) Repository: rC Clang https://reviews.llvm.org/D45639 ___ cfe-commits mailing list

[PATCH] D50361: [NFC] Test automatic variable initialization

2018-08-07 Thread JF Bastien via Phabricator via cfe-commits
jfb added a comment. Further fixes in r339090 and r339093. Repository: rC Clang https://reviews.llvm.org/D50361 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50361: [NFC] Test automatic variable initialization

2018-08-06 Thread JF Bastien via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rC339089: [NFC] Test automatic variable initialization (authored by jfb, committed by ). Changed prior to commit: https://reviews.llvm.org/D50361?vs=159397=159448#toc Repository: rC Clang

[PATCH] D50361: [NFC] Test automatic variable initialization

2018-08-06 Thread JF Bastien via Phabricator via cfe-commits
jfb added a comment. I'm honestly not sure there's anything to review here since it's just showing us what the current behavior is. LMK if I'm not testing something that I should. I'd much rather test current behavior as one patch first, because then the follow-ups show a clear before / after

[PATCH] D50361: [NFC] Test automatic variable initialization

2018-08-06 Thread JF Bastien via Phabricator via cfe-commits
jfb created this revision. Herald added subscribers: cfe-commits, dexonsmith. r337887 started using memset for automatic variable initialization where sensible. A follow-up discussion leads me to believe that we should better test automatic variable initialization, and that there are probably

[PATCH] D47618: __c11_atomic_load's _Atomic can be const

2018-08-02 Thread JF Bastien via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rL338743: __c11_atomic_loads _Atomic can be const (authored by jfb, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.org/D47618 Files:

[PATCH] D49771: CodeGen: use non-zero memset when possible for automatic variables

2018-07-31 Thread JF Bastien via Phabricator via cfe-commits
jfb added a comment. In https://reviews.llvm.org/D49771#1183641, @mehdi_amini wrote: > > I'm worried, however, about generating a bunch more code than needed from > > clang in the hopes that the compiler will clean it up later. > > Isn't a strong design component of clang/LLVM? Clang does not

[PATCH] D49771: CodeGen: use non-zero memset when possible for automatic variables

2018-07-31 Thread JF Bastien via Phabricator via cfe-commits
jfb added a comment. In https://reviews.llvm.org/D49771#1183562, @mehdi_amini wrote: > In https://reviews.llvm.org/D49771#1183380, @jfb wrote: > > > In https://reviews.llvm.org/D49771#1181008, @mehdi_amini wrote: > > > > > I'm curious: isn't the kind of optimization we should expect LLVM to > >

[PATCH] D49771: CodeGen: use non-zero memset when possible for automatic variables

2018-07-31 Thread JF Bastien via Phabricator via cfe-commits
jfb added a comment. In https://reviews.llvm.org/D49771#1181008, @mehdi_amini wrote: > I'm curious: isn't the kind of optimization we should expect LLVM to provide? Maybe? It seems obvious to do here since we know we'll probably want to be doing it, and I have another patch I'm working on

[PATCH] D47618: __c11_atomic_load's _Atomic can be const

2018-07-27 Thread JF Bastien via Phabricator via cfe-commits
jfb marked an inline comment as done. jfb added a comment. Give the comments, I think this is ready to commit. Repository: rC Clang https://reviews.llvm.org/D47618 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D47618: __c11_atomic_load's _Atomic can be const

2018-07-27 Thread JF Bastien via Phabricator via cfe-commits
jfb updated this revision to Diff 157761. jfb added a comment. - Add constant AS pointer test. Repository: rC Clang https://reviews.llvm.org/D47618 Files: lib/Sema/SemaChecking.cpp test/Sema/atomic-ops.c test/SemaOpenCL/atomic-ops.cl Index: test/SemaOpenCL/atomic-ops.cl

[PATCH] D49771: CodeGen: use non-zero memset when possible for automatic variables

2018-07-24 Thread JF Bastien via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rL337887: CodeGen: use non-zero memset when possible for automatic variables (authored by jfb, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM

[PATCH] D49771: CodeGen: use non-zero memset when possible for automatic variables

2018-07-24 Thread JF Bastien via Phabricator via cfe-commits
jfb marked an inline comment as done. jfb added a comment. Addressed all comments. Comment at: lib/CodeGen/CGDecl.cpp:956-957 +class BytePattern { + uint8_t Val; + enum class ValueType { Specific, Any, None } Type; + BytePattern(ValueType Type) : Type(Type) {}

[PATCH] D49771: CodeGen: use non-zero memset when possible for automatic variables

2018-07-24 Thread JF Bastien via Phabricator via cfe-commits
jfb updated this revision to Diff 157191. jfb marked 2 inline comments as done. jfb added a comment. - Use short to test padding between array elements. - Define enum class storage type; swap order of if / else to make it more readable. Repository: rC Clang https://reviews.llvm.org/D49771

[PATCH] D49771: CodeGen: use non-zero memset when possible for automatic variables

2018-07-24 Thread JF Bastien via Phabricator via cfe-commits
jfb created this revision. jfb added a reviewer: dexonsmith. Herald added a subscriber: cfe-commits. Right now automatic variables are either initialized with bzero followed by a few stores, or memcpy'd from a synthesized global. We end up encountering a fair amount of code where memcpy of

[PATCH] D47618: __c11_atomic_load's _Atomic can be const

2018-07-19 Thread JF Bastien via Phabricator via cfe-commits
jfb added a subscriber: yaxunl. jfb added inline comments. Comment at: lib/Sema/SemaChecking.cpp:3361 +if ((Form != Load && Form != LoadCopy && AtomTy.isConstQualified()) || AtomTy.getAddressSpace() == LangAS::opencl_constant) { Diag(DRE->getLocStart(),

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

2018-07-18 Thread JF Bastien via Phabricator via cfe-commits
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:

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

2018-07-18 Thread JF Bastien via Phabricator via cfe-commits
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

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

2018-07-17 Thread JF Bastien via Phabricator via cfe-commits
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

2018-07-17 Thread JF Bastien via Phabricator via cfe-commits
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

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

2018-07-13 Thread JF Bastien via Phabricator via cfe-commits
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

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

2018-07-13 Thread JF Bastien via Phabricator via cfe-commits
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

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

2018-07-13 Thread JF Bastien via Phabricator via cfe-commits
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:

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

2018-07-12 Thread JF Bastien via Phabricator via cfe-commits
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:

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

2018-07-12 Thread JF Bastien via Phabricator via cfe-commits
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 ::

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

2018-07-12 Thread JF Bastien via Phabricator via cfe-commits
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

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

2018-07-11 Thread JF Bastien via Phabricator via cfe-commits
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:

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

2018-07-05 Thread JF Bastien via Phabricator via cfe-commits
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 =

[PATCH] D47290: [Sema] -Wformat-pedantic only for NSInteger/NSUInteger %zu/%zi on Darwin

2018-06-22 Thread JF Bastien via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rL335393: [Sema] -Wformat-pedantic only for NSInteger/NSUInteger %zu/%zi on Darwin (authored by jfb, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM

[PATCH] D47290: [Sema] -Wformat-pedantic only for NSInteger/NSUInteger %zu/%zi on Darwin

2018-06-09 Thread JF Bastien via Phabricator via cfe-commits
jfb added a comment. In https://reviews.llvm.org/D47290#1126443, @aaron.ballman wrote: > In https://reviews.llvm.org/D47290#1125028, @aaron.ballman wrote: > > > Okay, that's fair, but the vendor-specific type for my Windows example is > > spelled `DWORD`. I'm really worried that this special

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

2018-06-04 Thread JF Bastien via Phabricator via cfe-commits
jfb added a comment. In https://reviews.llvm.org/D46024#1121242, @rkirsling wrote: > FWIW, please note that this space-before-brace style is not specific to > WebKit; CppCoreGuidelines exhibits it as well: > >

[PATCH] D47613: Mark __c11_atomic_load as const

2018-06-01 Thread JF Bastien via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rL333776: Mark __c11_atomic_load as const (authored by jfb, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.org/D47613 Files:

[PATCH] D47557: Filesystem tests: un-confuse write time

2018-05-31 Thread JF Bastien via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rL333723: Filesystem tests: un-confuse write time (authored by jfb, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.org/D47557 Files:

[PATCH] D47557: Filesystem tests: un-confuse write time

2018-05-31 Thread JF Bastien via Phabricator via cfe-commits
jfb updated this revision to Diff 149398. jfb added a comment. - Add back directory test Repository: rCXX libc++ https://reviews.llvm.org/D47557 Files: test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp Index:

[PATCH] D47618: __c11_atomic_load's _Atomic can be const

2018-05-31 Thread JF Bastien via Phabricator via cfe-commits
jfb added a comment. Note that I don't touch the OpenCL __constant stuff, because the separate address space means this can be actually read-only, which means you can't cmpxchg for very wide reads. Repository: rC Clang https://reviews.llvm.org/D47618

[PATCH] D47618: __c11_atomic_load's _Atomic can be const

2018-05-31 Thread JF Bastien via Phabricator via cfe-commits
jfb created this revision. jfb added a reviewer: rsmith. Herald added a subscriber: cfe-commits. C++11 onwards specs the non-member functions atomic_load and atomic_load_explicit as taking the atomic by const (potentially volatile) pointer. C11, in its infinite wisdom, decided to drop the

[PATCH] D47613: Mark __c11_atomic_load as const

2018-05-31 Thread JF Bastien via Phabricator via cfe-commits
jfb created this revision. jfb added reviewers: EricWF, mclow.lists. Herald added subscribers: cfe-commits, christof. C++11 onwards specs the non-member functions atomic_load and atomic_load_explicit as taking the atomic by const (potentially volatile) pointer. C11, in its infinite wisdom,

[PATCH] D47557: Filesystem tests: un-confuse write time

2018-05-31 Thread JF Bastien via Phabricator via cfe-commits
jfb updated this revision to Diff 149377. jfb added a comment. - Remove access time checks, simplify existing check, after talking to EricWF on IRC. Repository: rCXX libc++ https://reviews.llvm.org/D47557 Files:

[PATCH] D47557: Filesystem tests: un-confuse write time

2018-05-30 Thread JF Bastien via Phabricator via cfe-commits
jfb created this revision. jfb added reviewers: EricWF, mclow.lists, aemerson. Herald added subscribers: cfe-commits, christof. The filesystem test was confused about access versus write / modification time. The spec says: file_time_type last_write_time(const path& p, error_code& ec)

[PATCH] D46084: [Fixed Point Arithmetic] Addition of the Fixed Point _Accum type

2018-05-30 Thread JF Bastien via Phabricator via cfe-commits
jfb added inline comments. Comment at: include/clang/Basic/TokenKinds.def:393 +// ISO/IEC JTC1 SC22 WG14 N1169 Extension +KEYWORD(_Accum , KEYALL) + ebevhan wrote: > I believe that having KEYALL will enable the keyword even if

[PATCH] D47290: [Sema] -Wformat-pedantic only for NSInteger/NSUInteger %zu/%zi on Darwin

2018-05-29 Thread JF Bastien via Phabricator via cfe-commits
jfb added a comment. In https://reviews.llvm.org/D47290#1113422, @aaron.ballman wrote: > In https://reviews.llvm.org/D47290#1113412, @jfb wrote: > > > In https://reviews.llvm.org/D47290#1113365, @aaron.ballman wrote: > > > > > This is relaxing `-Wformat` and making users instead write > > >

[PATCH] D47290: [Sema] -Wformat-pedantic only for NSInteger/NSUInteger %zu/%zi on Darwin

2018-05-26 Thread JF Bastien via Phabricator via cfe-commits
jfb added a comment. Addressed comments. Repository: rC Clang https://reviews.llvm.org/D47290 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47290: [Sema] -Wformat-pedantic only for NSInteger/NSUInteger %zu/%zi on Darwin

2018-05-26 Thread JF Bastien via Phabricator via cfe-commits
jfb updated this revision to Diff 148737. jfb marked 2 inline comments as done. jfb added a comment. - Fix variable capitalization. Repository: rC Clang https://reviews.llvm.org/D47290 Files: include/clang/Analysis/Analyses/FormatString.h include/clang/Basic/DiagnosticSemaKinds.td

<    1   2   3   4   5   6   >