[PATCH] D41387: Remove llvm::MemoryBuffer const_casts

labath created this revision. labath added reviewers: dblaikie, rsmith. llvm has grown a WritableMemoryBuffer class, which is convertible (inherits from) a MemoryBuffer. We can use it to avoid conts_casting the buffer contents when we want to write to it. Repository: rC Clang

[PATCH] D41387: Remove llvm::MemoryBuffer const_casts

This revision was automatically updated to reflect the committed changes. Closed by commit rL321167: Remove llvm::MemoryBuffer const_casts (authored by labath, committed by ). Repository: rL LLVM https://reviews.llvm.org/D41387 Files: cfe/trunk/lib/Basic/SourceManager.cpp

[PATCH] D46685: [CodeGen] Disable structor optimizations at -O0

This revision was automatically updated to reflect the committed changes. Closed by commit rC332228: [CodeGen] Disable aggressive structor optimizations at -O0 (authored by labath, committed by ). Changed prior to commit: https://reviews.llvm.org/D46685?vs=146309=146578#toc Repository: rC

[PATCH] D46685: [CodeGen] Disable structor optimizations at -O0

labath added a comment. Thank you for the quick review. Repository: rC Clang https://reviews.llvm.org/D46685 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46685: [CodeGen] Disable structor optimizations at -O0

labath added a comment. I've reverted the patch as it has caused miscompiles on non-trivial inputs. I'll update the patch when I gain a better understanding of what is going on. Repository: rC Clang https://reviews.llvm.org/D46685 ___

[PATCH] D46685: [CodeGen] Disable structor optimizations at -O0

labath created this revision. labath added reviewers: rjmccall, aprantl. Merging of complete and base structors can degrade debug quality as it will leave the debugger unable to locate the full object structor. This is apparent when evaluating an expression in the debugger which requires

[PATCH] D46685: [CodeGen] Disable structor optimizations at -O0

labath updated this revision to Diff 146309. labath added a comment. Yes we can do that. Only the RAUW optimization is nasty, I've checked that the debugger is able to use the alias with no problem. Updated the patch the reflect that. Repository: rC Clang https://reviews.llvm.org/D46685

[PATCH] D46685: [CodeGen] Disable structor optimizations at -O0

labath updated this revision to Diff 147315. labath added a comment. Updating with a new version of the patch. The problem with the previous version was that it caused us to use C5/https://reviews.llvm.org/D5 comdats very aggressively (at -O0), and this is not always correct. This uses a more

[PATCH] D46685: [CodeGen] Disable structor optimizations at -O0

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rC332839: [CodeGen] Disable aggressive structor optimizations at -O0, take 2 (authored by labath, committed by ). Changed

[PATCH] D47887: Move VersionTuple from clang/Basic to llvm/Support, llvm part

This revision was automatically updated to reflect the committed changes. Closed by commit rL334399: Move VersionTuple from clang/Basic to llvm/Support (authored by labath, committed by ). Changed prior to commit: https://reviews.llvm.org/D47887?vs=150339=150701#toc Repository: rL LLVM

[PATCH] D47886: Move VersionTuple from clang/Basic to llvm/Support, clang part

labath closed this revision. labath added a comment. Committed as a part of https://reviews.llvm.org/D47887 via monorepo. Repository: rC Clang https://reviews.llvm.org/D47886 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D47886: Move VersionTuple from clang/Basic to llvm/Support, clang part

labath created this revision. labath added reviewers: erik.pilkington, zturner. Herald added a subscriber: mgorny. This kind of functionality is useful to other project apart from clang. LLDB works with version numbers a lot, but it does not have a convenient abstraction for this. Moving this

[PATCH] D47887: Move VersionTuple from clang/Basic to llvm/Support, llvm part

labath created this revision. labath added reviewers: zturner, erik.pilkington. Herald added a subscriber: mgorny. This kind of functionality is useful to other project apart from clang. LLDB works with version numbers a lot, but it does not have a convenient abstraction for this. Moving this

[PATCH] D47887: Move VersionTuple from clang/Basic to llvm/Support, llvm part

labath added a comment. Yea, one day I'll have to try that out. I'm just putting it off cause it would nuke all my branches and build folders :/ FWIW, the llvm part can actually be committed without breaking clang or anyone. Repository: rL LLVM https://reviews.llvm.org/D47887

[PATCH] D48241: [DebugInfo] Emit ObjC methods as part of interface.

labath added a comment. I also don't want to get involved too much, but here are my 2c: :) In https://reviews.llvm.org/D48241#1134342, @JDevlieghere wrote: > Putting it in a separate column is also a bad idea, because that means the > column is present for all the entries, including the ones

[PATCH] D47532: [ASTImporter] Import the whole redecl chain of functions

labath added a comment. In https://reviews.llvm.org/D47532#1144966, @martong wrote: > The broken lldb tests are fixed with a minor change. We no longer load the > Decls from the > external source during the call of `DeclContext::containsDecl`. A new > function >

[PATCH] D47532: [ASTImporter] Import the whole redecl chain of functions

labath added subscribers: aprantl, labath. labath added a comment. This has broken the LLDB bot http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-cmake/builds/25132. Could you take a look? Repository: rC Clang https://reviews.llvm.org/D47532

[PATCH] D49989: WIP: [CodeGen] Disable aggressive structor optimizations at -O0, take 4

labath created this revision. Herald added subscribers: cfe-commits, aheejin. This version of the patch attempts to mitigate the code size regressions caused by the previous versions of this patch (https://reviews.llvm.org/D46685). It does this by using aliases and C5/https://reviews.llvm.org/D5

[PATCH] D49594: [DebugInfo] Emit diagnostics when enabling -fdebug-types-section on non-linux target.

labath added a comment. I believe debug_types is used on non-linux targets as well. Judging by the other review all ELF targets should at least have a chance of working, so maybe key the error off of that? Repository: rC Clang https://reviews.llvm.org/D49594

[PATCH] D46685: [CodeGen] Disable structor optimizations at -O0

labath added a comment. This was reverted in r333482 because it was causing "definition with same mangled name as another definition" errors in some internal google builds. It turned out this uncovered an (unrelated) issue in module importing. This has now been fixed (r336240), so I'm planning

[PATCH] D51576: Enable DWARF accelerator tables by default when tuning for lldb (-glldb => -gpubnames)

labath created this revision. labath added a reviewer: dblaikie. DWARF v5 accelerator tables provide a considerable performance improvement for lldb and will make the default -glldb behavior same on all targets (right now we emit apple tables on apple targets, but these are not controlled by

[PATCH] D51576: Enable DWARF accelerator tables by default when tuning for lldb (-glldb => -gpubnames)

labath added a comment. In https://reviews.llvm.org/D51576#1223596, @clayborg wrote: > We want to ensure that both Apple and DWARF5 tables never get generated > though. That would waste a lot of space. I would say if DWARF5 tables are > enabled, then we need ensure we disable Apple tables.

[PATCH] D51576: Enable DWARF accelerator tables by default when tuning for lldb (-glldb => -gpubnames)

labath added a comment. In https://reviews.llvm.org/D51576#1223234, @aprantl wrote: > This is DWARF5+ only, right? (We shouldn't change the preference of Apple > accelerator tables for DWARF 4 and earlier). The interactions here are a bit weird, but the short answer is no, this will not

[PATCH] D51576: Enable DWARF accelerator tables by default when tuning for lldb (-glldb => -gpubnames)

This revision was automatically updated to reflect the committed changes. Closed by commit rL341472: Enable DWARF accelerator tables by default when tuning for lldb (-glldb =… (authored by labath, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM

[PATCH] D56599: [Support] Remove error return value from one overload of fs::make_absolute

This revision was automatically updated to reflect the committed changes. Closed by commit rC351317: [Support] Remove error return value from one overload of fs::make_absolute (authored by labath, committed by ). Herald added a subscriber: cfe-commits. Changed prior to commit:

[PATCH] D51641: [VFS] Cache the current working directory for the real FS.

labath added subscribers: JDevlieghere, labath. labath added a comment. Might I ask what was the motivation for this change? Performance optimalization? I am asking this because this makes the RealFileSystem return bogus values for the CWD if it changes through means other than the

[PATCH] D51641: [VFS] Cache the current working directory for the real FS.

labath added a comment. In D51641#1352782 , @sammccall wrote: > > In fact, it still happens now, because the VFS is so bad at having a local > > CWD. So, the only reason we actually discovered this was because > > VFS->getCurrentWorkingDirectory

[PATCH] D51641: [VFS] Cache the current working directory for the real FS.

labath added a comment. I wholeheartedly support an openat(2) based VFS, as the current one falls short of the expectations you have of it and is pretty broken right now. As for your assumption #2, I think that depends on what does one want to get out of the VFS. I can certainly see a use for

[PATCH] D57330: Adjust documentation for git migration.

labath added a comment. I am not sure we should be recommending to people to place the build folder under the llvm-project checkout. Is that how people use the monorepo build nowadays? This is not an full out-of-source build, since the build folder is still a subfolder of the repo root (and

[PATCH] D59692: [ASTImporter] Fix name conflict handling with different strategies

labath added a comment. In D59692#1646990 , @martong wrote: > Jenkins is not green > http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/743/ > However, the newly failing test is `TestTargetCommand.py`, which seems to be > unrelated. That ought

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

labath added inline comments. Comment at: clang/test/Driver/vfsmode.py:4 + +# UNSUPPORTED: system-windows + jfb wrote: > 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 :-( Unsupported

[PATCH] D70764: build: reduce CMake handling for zlib

labath added a comment. `/usr/lib/x86_64-linux-gnu/libz.so` is definitely better than `-l/usr/lib/x86_64-linux-gnu/libz.so`, as it's at least a valid link line. It's not completely equivalent, and may not work in all cases -- last time I accidentally changed this, I got about a dozen emails

[PATCH] D71508: [DebugInfo] Duplicate file names in debug info

labath added a comment. In D71508#1785767 , @probinson wrote: > Do we have a similar problem if the filespec has an embedded ./ or ../ in it? > I'm thinking some broader canonicalization ought to be done here. > $ clang ./dir1/dir2/../dir3/file.c >

[PATCH] D70764: build: reduce CMake handling for zlib

labath added a comment. Please take a look at D70519 for the issues with this approach. Also, while I do agree with you that we should not auto-select dependencies, I think this runs contrary to the llvm philosophy that the default built should "just work" (I

[PATCH] D71707: clang-tidy: new bugprone-pointer-cast-widening

labath resigned from this revision. labath added a comment. Though this *was* my idea, I don't really feel qualified to review code here. However, some things to consider: - disallowing casts to intptr_t seems too restrictive -- I doubt many people are doing that, but I guess this type exists

[PATCH] D76801: [AST] Print a> without extra spaces in C++11 or later.

labath added a comment. In D76801#1993337 , @dblaikie wrote: > In D76801#1991904 , @labath wrote: > > > David's example does work with gdb without -Wl,--gdb-index (the member > > variable is shown), presumably due

[PATCH] D76801: [AST] Print a> without extra spaces in C++11 or later.

labath added a comment. David's example does work with gdb without -Wl,--gdb-index (the member variable is shown), presumably due to the aforementioned fuzzy matching. However, it does not work if gdb-index is enabled, nor with lldb (as lldb is always very index-oriented and assumes equality

[PATCH] D76801: [AST] Print a> without extra spaces in C++11 or later.

labath added a comment. In D76801#1997451 , @dblaikie wrote: > Yeah, points all taken - as for this actual issue... I'm kind of inclined to > say "hey, our template names already diverge somewhat - and this divergence > is in the realm of acceptable by

[PATCH] D76801: [AST] Print a> without extra spaces in C++11 or later.

labath added a comment. In D76801#1995058 , @dblaikie wrote: > > It becomes a gdb-index problem because with an index the debugger will do a > > (hashed?) string lookup and expect the string to be there. If the strings > > differ, the lookup won't find

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

labath added inline comments. Comment at: llvm/cmake/config-ix.cmake:514 + if(ZLIB_FOUND) +set(LLVM_ENABLE_ZLIB "YES" CACHE STRING + "Use zlib for compression/decompression if available. Can be ON, OFF, or FORCE_ON" JDevlieghere wrote: > phosek wrote:

[PATCH] D80001: [RFC/WIP][clang] Fix printing of names of inherited constructors

labath created this revision. labath added reviewers: rsmith, dblaikie. Herald added a subscriber: aprantl. Herald added a project: clang. This is a fairly hacky fix to the following problem: Debug information entries for inherited constructors are emitted with the name of the base class, instead

[PATCH] D87590: Backport D79219, D85820, D86134 to 10.0 branch

labath added subscribers: tstellar, labath. labath added a reviewer: tstellar. labath added a comment. @tstellar, who is (was) the release manager for 10.0, can make a definitive call, but our usual modus operandi is to have one point release for each major version. I don't know if this is a

[PATCH] D87590: Backport D79219, D85820, D86134 to 10.0 branch

labath added a comment. In D87590#2273721 , @haampie wrote: > Also note that the changes are not likely to be backported to 11 even: > https://reviews.llvm.org/rG31e5f7120bdd2f76337686d9d169b1c00e6ee69c#942622. 11.0.0, probably. There's should be enough

[PATCH] D86660: Modifying ImportDeclContext(...) to ensure that we also handle the case when the FieldDecl is an ArrayType whose ElementType is a RecordDecl

labath added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:1755-1759 + QualType FromTy = ArrayFrom->getElementType(); + QualType ToTy = ArrayTo->getElementType(); + + FromRecordDecl = FromTy->getAsRecordDecl(); + ToRecordDecl =

[PATCH] D86660: Modifying ImportDeclContext(...) to ensure that we also handle the case when the FieldDecl is an ArrayType whose ElementType is a RecordDecl

labath added a comment. I'm going to respond to the rest of your (very insightful) comment later. So far, I'm just responding to this: >> This isn't exactly layout related, but there is the question of covariant >> methods. If a method is covariant, then its return type must be complete. > >

[PATCH] D86660: Modifying ImportDeclContext(...) to ensure that we also handle the case when the FieldDecl is an ArrayType whose ElementType is a RecordDecl

labath added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:1755-1759 + QualType FromTy = ArrayFrom->getElementType(); + QualType ToTy = ArrayTo->getElementType(); + + FromRecordDecl = FromTy->getAsRecordDecl(); + ToRecordDecl =

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

labath added a comment. I wouldn't mind separate (internal) cache variable, though I am somewhat surprised by this problem. A (non-cache) variable set in one of the parent scopes should still take precedence over a cache variable with the same name. And since config-ix.cmake is included from

[PATCH] D84554: Use INTERFACE_COMPILE_OPTIONS to disable -Wsuggest-override for any target that links to gtest

labath added a comment. In D84554#2176041 , @logan-5 wrote: > In D84554#2175012 , @labath wrote: > >> Could you elaborate on the lldb issue? I'd like to take a look at that... > > It looks like

[PATCH] D84554: Use INTERFACE_COMPILE_OPTIONS to disable -Wsuggest-override for any target that links to gtest

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. This looks good to me. Thanks. Could you elaborate on the lldb issue? I'd like to take a look at that... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D116329: [clang-check] Adjust argument adjusters for clang-check to strip options blocking the static analyzer

labath resigned from this revision. labath added a comment. I'm sorry, but I don't feel qualified to review this. Last time I saw this file was in 2013. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116329/new/ https://reviews.llvm.org/D116329

[PATCH] D116113: Add LLDB synthetic child and summary scripts for llvm::SmallVector, llvm::Optional, llvm::ErrorOr and llvm::Expected.

labath added a comment. Seems fairly straight-forward (and definitely useful). Just a couple of quick comments. Comment at: clang/utils/ClangDataFormat.py:28-35 +debugger.HandleCommand("type summary add -F ClangDataFormat.Optional_summary -x 'llvm::Optional<.*>'") +

[PATCH] D119910: [clang][debug] port clang-cl /JMC flag to ELF

labath added a comment. Not my usual wheelhouse, but since I'm here, here are some of my thoughts: - reusing the existing solution/code seems like a good idea -- no need to reinvent the wheel - how does that work, actually? How does it differentiate between "my" code and "code that happens to

[PATCH] D117744: [Driver] Remove obsoleted -gz=zlib-gnu

labath added a comment. This (unsurprisingly) broke an lldb test for the gnu style compression. You didn't get the notification because the bot was already red at the time. I've already fixed the situation by converting the test to yaml, but I thought you may want to know what happened.

[PATCH] D124606: Use `-text` git attribute instead of `text eol=...'

labath added inline comments. Comment at: clang-tools-extra/test/.gitattributes:7-15 +clang-apply-replacements/ClangRenameClassReplacements.cpp -text +clang-apply-replacements/Inputs/basic/basic.h -text +clang-apply-replacements/Inputs/format/no.cpp -text

[PATCH] D124606: Use `binary` git attribute instead of `text eol=...'

labath added a comment. I believe (based on some similar issues I was solving in lldb) that `-text` instead of `binary` is sufficient to prevent git from fidlling with the contents, while maintaining the ability to diff the files. Other than that, I am in favour of this approach, but I'd

[PATCH] D124606: Use `-text` git attribute instead of `text eol=...'

labath added a comment. In D124606#3480164 , @aaronpuchert wrote: > Fair enough, but don't we want to enforce LF or CRLF, respectively? Sure, but is the version control system the right tool to do that? I think it'd be better to have the test itself

[PATCH] D131007: [NFCI] Refactor how KeywordStatus is calculated

labath added a comment. This seems to break lldb tests, specifically the compilation of this source file. The compile fails with an assertion (`Keyword not known to come from a newer Standard

[PATCH] D144181: [clang][DebugInfo] Add abi-tags on constructors/destructors as LLVM annotations

labath added a comment. In D144181#4142295 , @Michael137 wrote: > We thought a bit about what it would take to link a constructor declaration > DIE to the various definitions (e.g., via a > `DW_AT_LLVM_complete_ctor_linkage_name` or

[PATCH] D144181: [clang][DebugInfo] Add abi-tags on constructors/destructors as LLVM annotations

labath added a comment. In D144181#4140831 , @Michael137 wrote: >> The construction of the mangled name does not require getting just the abi >> tags of the constructor itself right. Rather, it requires knowing the abi >> tag annotations of all

[PATCH] D144181: [clang][DebugInfo] Add abi-tags on constructors/destructors as LLVM annotations

labath added a comment. In D144181#4139446 , @Michael137 wrote: >> Hmm, I'd sort of still be inclined to look further at the linkage name >> option - but maybe the existence of a situation where the usage is built >> with debug info, but the out of