Re: r354843 - [CGDebugInfo] Set NonTrivial DIFlag to a c++ record if it's not trivial

Hi Aaron, I don't see any mention of this in D44406 - so it might have been good to have a separate review for this (or included this in the review of D44406, which I think is possible with the monorepo). Specifically - this change is missing test coverage (there should be a clang test that goes

Re: r350856 - Split -Wdelete-non-virtual-dtor into two groups

Might be handy to summarize the changes from the previous reverted version of this patch (& mention the original commit revision and revert revision) - in the commit message is ideal, but in a reply to the commit after the fact will do in a pinch On Fri, Jan 11, 2019 at 4:06 AM Erik Pilkington

Re: r350143 - Add vtable anchor to classes.

While I realize it's in the coding standard - is there any particular other motivation for this? (Given you've been doing layering cleanup - I'm wondering fi this is an interesting workaround for some layering problems, for instance?) On Sat, Dec 29, 2018 at 1:05 PM Richard Trieu via cfe-commits

r349669 - PR40096: Forwards-compatible with C++20 rule regarding aggregates not having user-declared ctors

Author: dblaikie Date: Wed Dec 19 11:33:35 2018 New Revision: 349669 URL: http://llvm.org/viewvc/llvm-project?rev=349669=rev Log: PR40096: Forwards-compatible with C++20 rule regarding aggregates not having user-declared ctors Looks like these were in place to make these types move-only. That's

Re: r346789 - DebugInfo: Add a driver flag for DWARF debug_ranges base address specifier use.

(tiny factoid: turns out gold's 64 bit gdb-index support is also broken in a different, more subtle way... - so best folks don't use this flag except with lld, it seems, if you're using gdb-index) On Tue, Nov 13, 2018 at 12:10 PM David Blaikie via cfe-commits < cfe-commits@lists.llvm.org>

Re: r348685 - Move diagnostic enums into Basic.

Hey Richard, Thanks for cleaning up some of the layering here! I /think/ I vaguely recall having a conversation with Richard Smith about a different direction to fix the layering of the diagnostics system - but it was/is more involved. Ah, here, apparently I sent out a WIP patch & must've got

Re: [PATCH] D55377: Allow forwarding -fdebug-compilation-dir to cc1as

Would it be worth considering whether -fdebug-compilation-dir and -fdebug-prefix-map could be unified, perhaps by having a placeholder that could be used in -fdebug-prefix-map for the current directory? I guess they're low-maintenance enough that it's probably not, but figured I'd ask. On Thu,

Re: r348131 - [AST] Fix an uninitialized bug in the bits of FunctionDecl

Ah, thanks for the explanation! No worries about pre-commit review or anything - this is what post-commit review is :) Only note for the future is that it might be worth mentioning in the body of the commit message (title/first line was fine) so it's clear why this "extra" work is being done.

Re: r348131 - [AST] Fix an uninitialized bug in the bits of FunctionDecl

Why the change from using setter functions to direct assignment? On Mon, Dec 3, 2018 at 5:06 AM Bruno Ricci via cfe-commits < cfe-commits@lists.llvm.org> wrote: > Author: brunoricci > Date: Mon Dec 3 05:04:10 2018 > New Revision: 348131 > > URL:

Re: r347588 - Revert "[clang][slh] add attribute for speculative load hardening"

Also, including the SVN revision (llvm's util/git-svn/git-svnrevert script can help with this) is helpful for other folks following along who may not be using git. On Mon, Nov 26, 2018 at 12:19 PM Aaron Ballman via cfe-commits < cfe-commits@lists.llvm.org> wrote: > On Mon, Nov 26, 2018 at 3:13

Re: r347035 - [clang] - Simplify tools::SplitDebugName.

Just copying some of the information given through Phab ( https://reviews.llvm.org/rL347035 ) - this patch was reverted in r347676. Would be great to get that fixed/recommitted! On Fri, Nov 16, 2018 at 12:01 AM George Rimar via cfe-commits < cfe-commits@lists.llvm.org> wrote: > Author: grimar >

r347141 - Sink BuryPointer from Clang into LLVM for reuse there

Author: dblaikie Date: Sat Nov 17 10:04:13 2018 New Revision: 347141 URL: http://llvm.org/viewvc/llvm-project?rev=347141=rev Log: Sink BuryPointer from Clang into LLVM for reuse there Modified: cfe/trunk/include/clang/Frontend/CompilerInstance.h cfe/trunk/include/clang/Frontend/Utils.h

r346929 - NFC cleanup: Prefer make_unique over reset(new T())

Author: dblaikie Date: Wed Nov 14 19:04:23 2018 New Revision: 346929 URL: http://llvm.org/viewvc/llvm-project?rev=346929=rev Log: NFC cleanup: Prefer make_unique over reset(new T()) Modified: cfe/trunk/lib/Parse/ParsePragma.cpp Modified: cfe/trunk/lib/Parse/ParsePragma.cpp URL:

r346926 - Stmt bits: Make ExprBits relative to StmtBits

Author: dblaikie Date: Wed Nov 14 19:04:18 2018 New Revision: 346926 URL: http://llvm.org/viewvc/llvm-project?rev=346926=rev Log: Stmt bits: Make ExprBits relative to StmtBits Seems like it makes it a bit easier to read/validate/update in the future. Modified:

r346927 - Rewrite-imports on crash: Simplify handling

Author: dblaikie Date: Wed Nov 14 19:04:19 2018 New Revision: 346927 URL: http://llvm.org/viewvc/llvm-project?rev=346927=rev Log: Rewrite-imports on crash: Simplify handling -frewrite-imports already implies -frewrite-includes (it piggy-backs on/extends the implementation) so there's no need to

r346928 - Fix combining pragma __debug dump & parser_crash with -E

Author: dblaikie Date: Wed Nov 14 19:04:21 2018 New Revision: 346928 URL: http://llvm.org/viewvc/llvm-project?rev=346928=rev Log: Fix combining pragma __debug dump & parser_crash with -E Previously these would be transformed into annotation tokens and the preprocessor would then assume they were

r346789 - DebugInfo: Add a driver flag for DWARF debug_ranges base address specifier use.

Author: dblaikie Date: Tue Nov 13 12:08:13 2018 New Revision: 346789 URL: http://llvm.org/viewvc/llvm-project?rev=346789=rev Log: DebugInfo: Add a driver flag for DWARF debug_ranges base address specifier use. Summary: This saves a lot of relocations in optimized object files (at the cost of

Re: r346266 - Don't use std::next() on an input iterator; NFC.

Thanks! On Mon, Nov 12, 2018 at 2:14 PM Aaron Ballman wrote: > On Mon, Nov 12, 2018 at 1:30 PM David Blaikie wrote: > > > > The previous code didn't have a conditional for Iter != End - was that a > bug? Should there be a test case for that bug? > > > > If that's not an actual change in

Re: r346266 - Don't use std::next() on an input iterator; NFC.

The previous code didn't have a conditional for Iter != End - was that a bug? Should there be a test case for that bug? If that's not an actual change in behavior, could it be an assert instead of a condition? On Tue, Nov 6, 2018 at 1:14 PM Aaron Ballman via cfe-commits <

r346439 - [Frontend/Modules] Show diagnostics on prebuilt module configuration mismatch too

Author: dblaikie Date: Thu Nov 8 12:47:30 2018 New Revision: 346439 URL: http://llvm.org/viewvc/llvm-project?rev=346439=rev Log: [Frontend/Modules] Show diagnostics on prebuilt module configuration mismatch too The current version only emits the below error for a module (attempted to be

Re: r342827 - Fix modules build with shared library.

Shuai - have you had a chance to look at this? On Mon, Oct 22, 2018 at 4:43 PM Richard Smith wrote: > On Mon, 22 Oct 2018 at 14:57, David Blaikie via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > >> Richard - any further thoughts here (re: layering/dependencies

Re: r345591 - [CodeGen] Disable the machine verifier on a ThinLTO test

If ThinLTO doesn't pass the machine verifier - should it maybe be turned off at the thinlto level in general, rather than for this specific test? On Tue, Oct 30, 2018 at 5:20 AM Francis Visoiu Mistrih via cfe-commits < cfe-commits@lists.llvm.org> wrote: > Author: thegameg > Date: Tue Oct 30

Re: r345695 - Change "struct" to "class" to avoid warnings

Could you link to/quote the warnings - might be helpful to understanding what's being addressed here On Tue, Oct 30, 2018 at 10:00 PM Bill Wendling via cfe-commits < cfe-commits@lists.llvm.org> wrote: > Author: void > Date: Tue Oct 30 21:58:34 2018 > New Revision: 345695 > > URL:

Re: r345109 - Debug Info (-gmodules): emit full types for non-anchored template specializations

Fair enough - pity we couldn't readily have a single implementation or at least semantics for modular debug info between implicit and explicit modes (I mean, my fault in part for building a separate/new system when I did modular codegen anyway) but hopefully we'll move to explicit modules across

Re: r344957 - Give Multiversion-inline functions linkonce linkage

On Mon, Oct 29, 2018 at 11:46 AM Keane, Erich wrote: > > > > > *From:* David Blaikie [mailto:dblai...@gmail.com] > *Sent:* Monday, October 29, 2018 11:41 AM > *To:* Keane, Erich > *Cc:* Eric Christopher ; cfe-commits@lists.llvm.org > > > *Subject:* Re: r344957 - Give Multiversion-inline

Re: r344957 - Give Multiversion-inline functions linkonce linkage

On Mon, Oct 29, 2018 at 11:30 AM Keane, Erich wrote: > GCC actually doesn’t support function multiversioning in C mode, so this > fix is part of our extension to support C with multiversioning. > Ah, what's the motivation for that? > I perhaps wonder if this is part of the reason GCC only

Re: r345109 - Debug Info (-gmodules): emit full types for non-anchored template specializations

Is this a workaround for now with the intent to fix this to allow such implicit specializations to have their debug info modularized? I believe this does work correctly in modular debug info with expliict modules, would probably be sort of nice to have these things be consistent/similar? On Tue,

Re: r344957 - Give Multiversion-inline functions linkonce linkage

Does this match GCC's approach here? (I ask this sort of as throwaway/conversation starter - because the linkage/behavior around multiversion functions and their inlining is full of sharp corners/risks of code moving out of the areas appropriately restricted based on the cpu features) On Mon,

Re: r342827 - Fix modules build with shared library.

re in lib/StaticAnalyzer. There shouldn't >> be any problem with clang-tidy using it from there, since it already >> depends on the static analyzer. >> > I'm happy to do the move. > Could you (or someone) help point out where exactly I should move it to > though

Re: [PATCH] D38061: Set AnonymousTagLocations false for ASTContext if column info is expected not to be used

On Fri, Oct 19, 2018 at 3:56 PM Adrian Prantl via Phabricator via llvm-commits wrote: > aprantl added a comment. > > I have a vague recollection that this column info hack was added to > disambiguate two types defined on the same line (which is something that > happened more often than one would

Re: r342827 - Fix modules build with shared library.

I can't really reproduce this - when I try to build clang/llvm with LLVM_ENABLE_MODULES in CMake I'm still seeing an error I reported March on a cfe-dev thread - something to do with unique_ptr instantiations for MappedBlockStream in the PDB parsing code. So, I'm wondering what error you hit,

Re: r343279 - [cxx2a] P0624R2: Lambdas with no capture-default are

Excellent, excellent - thanks! On Mon, Oct 1, 2018 at 10:22 AM Richard Smith wrote: > On Mon, 1 Oct 2018, 09:55 David Blaikie via cfe-commits, < > cfe-commits@lists.llvm.org> wrote: > >> Awesome - should/does this mean stateless lambdas can be used in >> uninitializ

Re: r343279 - [cxx2a] P0624R2: Lambdas with no capture-default are

Awesome - should/does this mean stateless lambdas can be used in uninitialized contexts? std::set s; Would be kind of neat/handy (so you didn't have to pass in the comparator on construction, etc) On Thu, Sep 27, 2018 at 3:48 PM Richard Smith via cfe-commits < cfe-commits@lists.llvm.org> wrote:

Re: r342912 - [CodeGen] Revert commit https://reviews.llvm.org/rL342717

When reverting patches it's helpful to include some context (links to buildbots, plus inline/textual descriptions of errors, etc) for why the change is being made - so people looking to explain the failure they're seeing can see if this change is intended to address it, or if someone's going back

Re: r343224 - [Tooling] Get rid of uses of llvm::Twine::str which is slow. NFC

On Thu, Sep 27, 2018 at 7:51 AM Eric Liu via cfe-commits < cfe-commits@lists.llvm.org> wrote: > Author: ioeric > Date: Thu Sep 27 07:50:24 2018 > New Revision: 343224 > > URL: http://llvm.org/viewvc/llvm-project?rev=343224=rev > Log: > [Tooling] Get rid of uses of llvm::Twine::str which is slow.

Re: r342827 - Fix modules build with shared library.

+Shuai Wang On Tue, Sep 25, 2018 at 2:14 PM David Blaikie wrote: > Hey Eric - thanks for the fix - but could you explain the issue here in a > bit more detail, as I'm a bit confused (& really interested in > understanding any layering problems in LLVM - and fixing them/making sure > they're

Re: r342827 - Fix modules build with shared library.

Hey Eric - thanks for the fix - but could you explain the issue here in a bit more detail, as I'm a bit confused (& really interested in understanding any layering problems in LLVM - and fixing them/making sure they're fixed/holding the line/etc) What do you mean by "pull all of the AST matchers

Re: [PATCH] D52191: Fix logic around determining use of frame pointer with -pg.

Fixed in r342510 with the solution I mentioned up-thread. On Tue, Sep 18, 2018 at 1:10 PM Volodymyr Sapsai via Phabricator < revi...@reviews.llvm.org> wrote: > vsapsai added a comment. > > Confirm that reverting the change locally fixes the tests. If nobody beats > me to it, I plan to revert the

r342510 - Fix fomit-frame-pointe+pg error

Author: dblaikie Date: Tue Sep 18 13:11:45 2018 New Revision: 342510 URL: http://llvm.org/viewvc/llvm-project?rev=342510=rev Log: Fix fomit-frame-pointe+pg error Modified: cfe/trunk/lib/Driver/ToolChains/Clang.cpp Modified: cfe/trunk/lib/Driver/ToolChains/Clang.cpp URL:

Re: r342214 - remove 11 years old videos from the homepage. if you have a suggestion, please drop me an email

If you're going to remove these, might as well remove the html files they link to? On the other hand, might be worth keeping these around/linked to, but for posterity/archival purposes, rather than as sources of legitimately useful information for people trying to use LLVM today. On Fri, Sep 14,

Re: r342165 - Support -fno-omit-frame-pointer with -pg.

Seems like it might be problematic to have this separate implementation of checking whether frame pointers are enabled compared to the canonical one (the one actually used to enable/disable frame pointers) in the static "shouldUseFramePointer" Function? (eg: apparently on some targets

r340206 - DebugInfo: Add the ability to disable DWARF name tables entirely

Author: dblaikie Date: Mon Aug 20 13:14:08 2018 New Revision: 340206 URL: http://llvm.org/viewvc/llvm-project?rev=340206=rev Log: DebugInfo: Add the ability to disable DWARF name tables entirely This changes the current default behavior (from emitting pubnames by default, to not emitting them by

r339968 - Disable pubnames in NVPTX debug info using metadata

Author: dblaikie Date: Thu Aug 16 16:56:32 2018 New Revision: 339968 URL: http://llvm.org/viewvc/llvm-project?rev=339968=rev Log: Disable pubnames in NVPTX debug info using metadata Added: cfe/trunk/test/CodeGen/debug-nvptx.c Modified: cfe/trunk/lib/CodeGen/CGDebugInfo.cpp Modified:

r339941 - Update for LLVM API change

Author: dblaikie Date: Thu Aug 16 14:30:24 2018 New Revision: 339941 URL: http://llvm.org/viewvc/llvm-project?rev=339941=rev Log: Update for LLVM API change Modified: cfe/trunk/lib/CodeGen/CGDebugInfo.cpp Modified: cfe/trunk/lib/CodeGen/CGDebugInfo.cpp URL:

Re: [PATCH] D50154: [clangd] capitalize diagnostic messages

On Wed, Aug 8, 2018 at 5:00 AM Ilya Biryukov via Phabricator < revi...@reviews.llvm.org> wrote: > ilya-biryukov added a comment. > > In https://reviews.llvm.org/D50154#1191002, @dblaikie wrote: > > > What's the motivation for clangd to differ from clang here? > > > The presentation of diagnostics

Re: [PATCH] D50154: [clangd] capitalize diagnostic messages

On Tue, Aug 7, 2018 at 4:02 PM Alex L wrote: > On Tue, 7 Aug 2018 at 11:38, David Blaikie wrote: > >> >> >> On Tue, Aug 7, 2018 at 11:22 AM Alex L wrote: >> >>> On Tue, 7 Aug 2018 at 10:52, David Blaikie via cfe-commits < >>> cfe-commits@list

Re: r338301 - Avoid returning an invalid end source loc

*nod* Maybe consistency's enough for now. But yeah, if you can test whether the assertion fires (though for invalid locs, that usually means invalid code - and we don't have nice big repositories of weird invalid code - just the clang regression tests). On Tue, Aug 7, 2018 at 3:27 PM Stephen

Re: r338301 - Avoid returning an invalid end source loc

If it never comes up, maybe an assertion would suffice? (& if the assertion ever does fire - hey, we've found a test case to use) How'd you find this/what motivated you to make the change? On Tue, Aug 7, 2018 at 3:11 PM Stephen Kelly wrote: > > Hi David, > > I'm happy to add a test case, but I

Re: [PATCH] D50154: [clangd] capitalize diagnostic messages

On Tue, Aug 7, 2018 at 11:22 AM Alex L wrote: > On Tue, 7 Aug 2018 at 10:52, David Blaikie via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > >> >> >> On Tue, Aug 7, 2018 at 10:33 AM Alex Lorenz via Phabricator < >> revi...@reviews.llvm.o

Re: [PATCH] D50154: [clangd] capitalize diagnostic messages

On Tue, Aug 7, 2018 at 10:33 AM Alex Lorenz via Phabricator < revi...@reviews.llvm.org> wrote: > arphaman added a comment. > > In https://reviews.llvm.org/D50154#1191002, @dblaikie wrote: > > > What's the motivation for clangd to differ from clang here? (& if the > first > > letter is going to

Re: r338732 - [analyzer] Make RegionVector use const reference

Looks good! Though it may be useful in the future to describe, in the commit message, the motivation for a change - how'd you find this? What motivated you to make this particular fix just now, etc? ("identified using clang-tidy" or "spotted during post-commit review of change r", etc...) On

Re: r338467 - Avoid exposing name for range-based for '__range' variables in lifetime warnings.

Reckon there's a chance of improved diagnostic text in cases like this? Will users understand what the problem is/how to fix it when they read "temporary implicitly bound to local reference will be destroyed at the end of the full-expression" - feels very standard-ese-y to me? & I appreciate teh

Re: [PATCH] D50154: [clangd] capitalize diagnostic messages

What's the motivation for clangd to differ from clang here? (& if the first letter is going to be capitalized, should there be a period at the end? But also the phrasing of most/all diagnostic text isn't in the form of complete sentences, so this might not make sense) On Fri, Aug 3, 2018 at 1:44

Re: r338301 - Avoid returning an invalid end source loc

test case? On Mon, Jul 30, 2018 at 1:39 PM Stephen Kelly via cfe-commits < cfe-commits@lists.llvm.org> wrote: > Author: steveire > Date: Mon Jul 30 13:39:14 2018 > New Revision: 338301 > > URL: http://llvm.org/viewvc/llvm-project?rev=338301=rev > Log: > Avoid returning an invalid end source loc

Re: [PATCH] D49265: [Tooling] Add operator== to CompileCommand

Any chance this can/should be unit tested? (also, in general (though might not matter in this instance), symmetric operators like == should be implemented as non-members (though they can still be friends and if they are, can be defined inline in the class definition as a member could be), so any

Re: r336475 - Check returned type is valid before using it.

Thanks for the fix! Though maybe this isn't the best diagnostic experience - given that 'bar' isn't technically an overloaded function, but is a function template - worth doing something more precise here? (maybe not) Wonder if there's similar handling for other diagnostic cases that could be

Re: r336219 - Fix crash in clang.

On Mon, Jul 9, 2018 at 1:52 PM Zachary Turner wrote: > makeArrayRef() isn't necessary, but when I was first looking at this I had > to stare at the code for a bit to see that there was an implicit conversion > happening. So I put the makeArrayRef() just for the benefit of the person > reading

Re: r336219 - Fix crash in clang.

Did this fail on an existing regression test, or is there a need for more test coverage? (guessing it failed on existing tests) Also, is the makeArrayRef necessary? Looks like if the original code compiled (implicitly converting from vector to ArrayRef) then the new code wouldn't need a

r336020 - Spurious commit just to help Richard, because git is weird.

Author: dblaikie Date: Fri Jun 29 14:58:24 2018 New Revision: 336020 URL: http://llvm.org/viewvc/llvm-project?rev=336020=rev Log: Spurious commit just to help Richard, because git is weird. Modified: cfe/trunk/test/Modules/codegen.test Modified: cfe/trunk/test/Modules/codegen.test URL:

Re: [PATCH] D47953: [builtin] Add bitfield support for __builtin_dump_struct

Yeah, doesn't look like this code handles a value crossing the boundary of the size of the bitfield type (it's reading only the low bit). I suspect looking at the code that generates bitfield accesses would be useful - and/or maybe actually calling into that very code, rather than reimplementing

r335938 - DebugInfo: Add -gno-gnu-pubnames to allow disabling gnu-pubnames later in the command line

Author: dblaikie Date: Thu Jun 28 15:58:04 2018 New Revision: 335938 URL: http://llvm.org/viewvc/llvm-project?rev=335938=rev Log: DebugInfo: Add -gno-gnu-pubnames to allow disabling gnu-pubnames later in the command line Modified: cfe/trunk/include/clang/Driver/Options.td

Re: r335022 - Revert r335019 "Update NRVO logic to support early return (Attempt 2)"

Whenever possible, do include context for why a patch is being reverted in the commit message - buildbot links, but also potentially describing/copy-pasting inline whatever the problem(s) are. This makes it easier for people tracking the state of upstream to know whether they may need to pull in

r334778 - Modules: Fix implicit output file for .cppm to .pcm instead of stdout

Author: dblaikie Date: Thu Jun 14 16:09:06 2018 New Revision: 334778 URL: http://llvm.org/viewvc/llvm-project?rev=334778=rev Log: Modules: Fix implicit output file for .cppm to .pcm instead of stdout This code was introduced back in r178148, a change to introduce -module-file-info - which still

r333955 - Update for an LLVM header file move

Author: dblaikie Date: Mon Jun 4 14:23:29 2018 New Revision: 333955 URL: http://llvm.org/viewvc/llvm-project?rev=333955=rev Log: Update for an LLVM header file move Modified: cfe/trunk/lib/CodeGen/CGCall.cpp Modified: cfe/trunk/lib/CodeGen/CGCall.cpp URL:

Re: [PATCH] D47687: fix: [Bug 18971] - Missing -Wparentheses warning

Probably CC someone from apple here & ask about rdar://8678458 - they can look it up & provide the missing context. On Mon, Jun 4, 2018 at 8:17 AM Roman Lebedev via Phabricator < revi...@reviews.llvm.org> wrote: > lebedev.ri added reviewers: rjmccall, akyrtzi. > lebedev.ri added a comment. > >

Re: [PATCH] D47597: IRGen: Write .dwo files when -split-dwarf-file is used together with -fthinlto-index.

On Thu, May 31, 2018 at 11:20 AM Peter Collingbourne via Phabricator < revi...@reviews.llvm.org> wrote: > pcc created this revision. > pcc added reviewers: tejohnson, dblaikie. > Herald added subscribers: JDevlieghere, hiraditya, eraman, inglorion, > mehdi_amini. > >

Re: r333141 - Use zeroinitializer for (trailing zero portion of) large array initializers

Probably nice to mention in the commit message what the fix was (& if/where there was there a test added for it?) so readers don't have to try to eyeball diff this commit against the otherone. On Wed, May 23, 2018 at 4:45 PM Richard Smith via cfe-commits < cfe-commits@lists.llvm.org> wrote: >

Re: r332720 - Move #include manipulation code to new lib/Tooling/Inclusions.

On Fri, May 18, 2018 at 2:22 PM Eric Liu <ioe...@google.com> wrote: > Thanks a lot for looking into this Bruno! The fix looks promising; I'll > give it a try next week :D > > On Fri, May 18, 2018 at 10:33 PM David Blaikie via cfe-commits < > cfe-commits@lists.llvm.or

Re: r332720 - Move #include manipulation code to new lib/Tooling/Inclusions.

I haven't looked in detail here, but in general: Don't split an implementation and its headers into different notional libraries, as that breaks modular code generation (an implementation and its headers usually have circular dependencies - inline functions that call non-inline functions, and the

Re: r331536 - [NFC]Convert Class to use member initialization instead of inline.

On Mon, May 7, 2018 at 12:57 PM Keane, Erich wrote: > I don’t believe the member initialization for bitfields (of which all the > ‘0’ values are) happened until C++17, right? > Ah, fair point - I hadn't looked at the types, just what was visible in the patch. (& I don't

Re: r331536 - [NFC]Convert Class to use member initialization instead of inline.

Perhaps this should use non-static data member initializers instead? On Fri, May 4, 2018 at 9:23 AM Erich Keane via cfe-commits < cfe-commits@lists.llvm.org> wrote: > Author: erichkeane > Date: Fri May 4 09:19:53 2018 > New Revision: 331536 > > URL:

r330671 - Fix build break due to content moving from Scalar.h to InstCombine.h in LLVM

Author: dblaikie Date: Mon Apr 23 17:59:22 2018 New Revision: 330671 URL: http://llvm.org/viewvc/llvm-project?rev=330671=rev Log: Fix build break due to content moving from Scalar.h to InstCombine.h in LLVM Modified: cfe/trunk/lib/CodeGen/BackendUtil.cpp Modified:

Re: [PATCH] D45766: [Sema] Add -Wno-self-assign-overloaded

On Mon, Apr 23, 2018 at 4:12 PM John McCall wrote: > On Mon, Apr 23, 2018 at 6:32 PM, David Blaikie wrote: > >> On Mon, Apr 23, 2018 at 3:29 PM John McCall via Phabricator < >> revi...@reviews.llvm.org> wrote: >> >>> rjmccall added a comment. >>> >>> In

Re: [PATCH] D45766: [Sema] Add -Wno-self-assign-overloaded

On Mon, Apr 23, 2018 at 3:29 PM John McCall via Phabricator < revi...@reviews.llvm.org> wrote: > rjmccall added a comment. > > In https://reviews.llvm.org/D45766#1076176, @dblaikie wrote: > > > Is there anything else in the "-w" namespace other than the literal "-w" > so > > far? > > > No. This

Re: [PATCH] D45766: [Sema] Add -Wno-self-assign-overloaded

Is there anything else in the "-w" namespace other than the literal "-w" so far? I mean, I could imagine it might make more sense to default these warnings off & users can turn them on for non-test code, potentially? So "-Wnon-test" might make sense. On Mon, Apr 23, 2018 at 3:22 PM John McCall

Re: [PATCH] D45766: [Sema] Add -Wno-self-assign-overloaded

FWIW I don't fundamentalyl object to also having something like -wtest. Probably needs a better name though (unfortunately the double-negative gets confusing... - like you want to describe the set of diagnostics that should not be used in test code, so that as a group might be "-Wnon-test" but

Re: [PATCH] D45685: [Sema] Add -wtest global flag that silences -Wself-assign for overloaded operators.

On Mon, Apr 16, 2018 at 12:08 PM Roman Lebedev via Phabricator < revi...@reviews.llvm.org> wrote: > lebedev.ri added a comment. > > In https://reviews.llvm.org/D45685#1069040, @dblaikie wrote: > > > I'm not sure this is a practical direction to pursue - though perhaps > > others disagree. > > >

Re: [PATCH] D45685: [Sema] Add -wtest global flag that silences -Wself-assign for overloaded operators.

I'm not sure this is a practical direction to pursue - though perhaps others disagree. It's likely non-trivial to plumb a flag through most build systems to be applied only to test code (& likely would suppress the warning in headers only included in test code - so for example, in a header-only

Re: [PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)

On Tue, Apr 10, 2018 at 10:20 AM John McCall wrote: > Do you think they’re bad precedent? Somewhat, yes - though -Wparens is perhaps conflating a few cases too. I think the argument for the -Wparens for precedence is probably pretty good. But the argument for -Wparens for

Re: [PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)

On Tue, Apr 10, 2018 at 9:59 AM John McCall wrote: > Also, the standard for the static analyzer is not lower than it is for the > compiler. > > The static analyzer tries to catch a much larger class of bugs, but it > also tries very hard to make all the warnings it emits

Re: [PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)

Any good ideas on how to evaluate it, then? (in addition to/other than something like Google where we can track the diagnostic for - a few months?) On Tue, Apr 10, 2018 at 9:34 AM John McCall wrote: > Yeah, -Wself-assign in general is about catching a class of live-coding >

Re: [PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)

On Tue, Apr 10, 2018 at 9:25 AM Roman Lebedev wrote: > Because *so far* it has been running on the existing code, which i guess > was already pretty warning free, and was run through the static analyzer, which obviously should catch such issues. > Existing code this has

Re: [PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)

It's more the fact that that's /all/ the warning improvement has found so far. If it was 8 false positives & also found 80 actionable bugs/bad code, that'd be one thing. Now, admittedly, maybe it would help find bugs that people usually catch with a unit test, etc but never make it to checked in

Re: [PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)

On Tue, Apr 10, 2018 at 7:21 AM Roman Lebedev via Phabricator < revi...@reviews.llvm.org> wrote: > lebedev.ri added a comment. > > In https://reviews.llvm.org/D44883#1063003, @thakis wrote: > > > This landing made our clang trunk bots do an evaluation of this warning > :-P It fired 8 times, all

Re: [PATCH] D43779: [Tooling] [0/1] Refactor FrontendActionFactory::create() to return std::unique_ptr<>

FWIW - I had some thoughts on this a while back: https://reviews.llvm.org/D4313 On Mon, Apr 9, 2018 at 4:54 AM Roman Lebedev via Phabricator via llvm-commits wrote: > lebedev.ri added a comment. > > In https://reviews.llvm.org/D43779#1061444, @alexfh wrote: > > > In

Re: r329445 - Revert "[analyzer] Remove an unused variable"

Best if you can use the SubVersion revision number rather than a git hash when reverting. There's a utility in the LLVM project to help with this (llvm/utils/git-svn/git-svnrevert) On Fri, Apr 6, 2018 at 12:16 PM George Karpenkov via cfe-commits < cfe-commits@lists.llvm.org> wrote: > Author:

r329097 - Restrict a test using named file descriptors to using the system shell

Author: dblaikie Date: Tue Apr 3 11:22:14 2018 New Revision: 329097 URL: http://llvm.org/viewvc/llvm-project?rev=329097=rev Log: Restrict a test using named file descriptors to using the system shell Modified: cfe/trunk/test/Misc/dev-fd-fs.c Modified: cfe/trunk/test/Misc/dev-fd-fs.c URL:

Re: [PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)

On Mon, Apr 2, 2018 at 8:05 AM Roman Lebedev via Phabricator < revi...@reviews.llvm.org> wrote: > lebedev.ri added a comment. > > In https://reviews.llvm.org/D44883#1054326, @thakis wrote: > > > In https://reviews.llvm.org/D44883#1048751, @dblaikie wrote: > > > > > Historically Clang's policy on

r328718 - Fix for LLVM header changes

Author: dblaikie Date: Wed Mar 28 10:45:10 2018 New Revision: 328718 URL: http://llvm.org/viewvc/llvm-project?rev=328718=rev Log: Fix for LLVM header changes Modified: cfe/trunk/lib/CodeGen/BackendUtil.cpp Modified: cfe/trunk/lib/CodeGen/BackendUtil.cpp URL:

Re: [PATCH] D44883: [Sema] Extend -Wself-assign with -Wself-assign-overloaded to warn on overloaded self-assignment (classes)

Historically Clang's policy on warnings was, I think, much more conservative than it seems to be today. There was a strong desire not to implement off-by-default warnings, and to have warnings with an exceptionally low false-positive rate - maybe the user-defined operator detection was either

r328380 - Change for an LLVM header file move

Author: dblaikie Date: Fri Mar 23 15:16:59 2018 New Revision: 328380 URL: http://llvm.org/viewvc/llvm-project?rev=328380=rev Log: Change for an LLVM header file move Modified: cfe/trunk/lib/CodeGen/BackendUtil.cpp Modified: cfe/trunk/lib/CodeGen/BackendUtil.cpp URL:

Re: [PATCH] D44826: Add -Wunused-using, a warning that finds unused using declarations.

While implementing the warning is great (wonder if there's any codebase that isn't -Wunused-using clean, that we could use to compare Clang and GCC's behavior broadly - make sure it's catching the same cases (or justify/investigate differences)) - and using it to motivate the debug info is an

Re: [PATCH] D44788: Add an option to support debug fission on implicit ThinLTO.

The only data we have is that where .o files go, .dwo files go beside them. How to generalize this to other situations isn't really obvious to me - even for a.out (do you put all the .dwo files next to a.out? in the same directory? if the names collide, where then? etc). Interestingly GCC for

Re: [PATCH] D44788: Add an option to support debug fission on implicit ThinLTO.

In implicit ThinLTO, the object files are only temporary. Sort of similar to using -gsplit-dwarf when compiling straight to an executable (without using -c): "clang++ x.cpp y.cpp -o a.out" - where should the .dwo files go then? If they go where the .o files go, then they'll be in /tmp/ and get

r328166 - Fix for LLVM change (Transforms/Utils/Local.h -> Analysis/Utils/Local.h)

Author: dblaikie Date: Wed Mar 21 15:34:27 2018 New Revision: 328166 URL: http://llvm.org/viewvc/llvm-project?rev=328166=rev Log: Fix for LLVM change (Transforms/Utils/Local.h -> Analysis/Utils/Local.h) Modified: cfe/trunk/lib/CodeGen/CGCall.cpp Modified: cfe/trunk/lib/CodeGen/CGCall.cpp

Re: r326324 - [analyzer] Fix a compiler warning

Might be useful in the future to use a more specific commit message such as "add missing override" (or "add missing override to address -Wmissing-override warning"), etc. On Wed, Feb 28, 2018 at 6:03 AM Gabor Horvath via cfe-commits < cfe-commits@lists.llvm.org> wrote: > Author: xazax > Date:

r325692 - Remove use of the 'gmlt' term from the -fsplit-dwarf-inlining flag description to make it more readily legible

Author: dblaikie Date: Wed Feb 21 08:00:50 2018 New Revision: 325692 URL: http://llvm.org/viewvc/llvm-project?rev=325692=rev Log: Remove use of the 'gmlt' term from the -fsplit-dwarf-inlining flag description to make it more readily legible Modified:

r325594 - PR36442: Correct description of -fsplit-dwarf-inlining

Author: dblaikie Date: Tue Feb 20 08:35:08 2018 New Revision: 325594 URL: http://llvm.org/viewvc/llvm-project?rev=325594=rev Log: PR36442: Correct description of -fsplit-dwarf-inlining Modified: cfe/trunk/include/clang/Driver/Options.td Modified: cfe/trunk/include/clang/Driver/Options.td

Re: [PATCH] D43259: Implement function attribute artificial

Maybe - though that'd actually make for larger debug info & not be much use. With nodebug+always_inline (which is how the intrinsics are provided) the function call dissolves into the raw instruction it's meant to represent. The debug info describes that instruction as if it had been written at

Re: r324498 - [Driver] Add option to manually control discarding value names in LLVM IR.

Ah.. hrm :/ On Mon, Feb 12, 2018 at 6:03 PM Eric Fiselier wrote: > On Mon, Feb 12, 2018 at 4:01 PM, David Blaikie wrote: > >> ah, sweet :) >> >> On Mon, Feb 12, 2018 at 2:59 PM Eric Fiselier wrote: >> >>> On Mon, Feb 12, 2018 at 3:35 PM, David

<    2   3   4   5   6   7   8   9   10   11   >