User Details
- User Since
- Jul 12 2017, 1:23 AM (253 w, 5 d)
Fri, May 13
Thanks, that explanation looks fine. (And I agree that re-paragraphing it made more sense than my version)
Tue, May 3
Apr 11 2022
Gentle ping: @rnk, are you satisfied with my changes and explanations from last week?
Apr 5 2022
Oh yes, so there is. Moved the def.
Fixed a CI failure (unreachable in tokenizeWindowsCommandLineImpl was reached because I forgot to correctly handle an unquoted initial backslash in CommandName mode), and added an extra unit test to ensure the triggering case stays working.
Apr 4 2022
Updated to fix the handling of a trailing unclosed quoted string, as suggested. It turns out (checking with that same 6-line test program) that even an empty unclosed quoted string still turns into an argv word, contrary to one of the existing unit tests.
Apr 1 2022
Mar 21 2022
Whereas I'm familiar with this code but not with the opaque-pointers effort, so I had to look up the other half of what was going on :-)
Mar 18 2022
Mar 7 2022
I like this version! This definitely says to me "nobody is going to just thoughtlessly append to an existing file".
Mar 3 2022
While splitting up the test file is not ideal [...]
Mar 2 2022
Feb 16 2022
LGTM this time. Thanks very much for the rework!
Feb 15 2022
I ran another test of this patch, by trying the cross product of the following cmake configuration choices (on a build targeting Windows, with LLVM_ENABLE_PROJECTS=clang):
Feb 14 2022
I gave this patch a test against our downstream code, and found that it doesn't stop me needing the change I suggested in D119199: in clang/tools/driver/CMakeLists.txt, I had to change
if(CLANG_PLUGIN_SUPPORT) export_executable_symbols_for_plugins(clang) endif()
so that the if statement reads
if(CLANG_PLUGIN_SUPPORT OR LLVM_EXPORT_SYMBOLS_FOR_PLUGINS)
Feb 11 2022
Hi. This commit seems to have broken the following cmake command, on Windows (with HEAD pointing at 76cad51ba700233d6e3492eddcbb466b6adbc2eb):
cmake -DLLVM_ENABLE_PROJECTS=clang -DLLVM_EXPORT_SYMBOLS_FOR_PLUGINS=ON ..\llvm-project\llvm
The symptom is the same as @cristian.adam shows above:
-- SampleAnalyzerPlugin ignored -- Loadable modules not supported on this platform. CMake Error at [...]/llvm-project/clang/cmake/modules/AddClang.cmake:185 (target_link_libraries): Utility target "SampleAnalyzerPlugin" must not be used as the target of a target_link_libraries call. Call Stack (most recent call first): [...]/llvm-project/clang/lib/Analysis/plugins/SampleAnalyzer/CMakeLists.txt:8 (clang_target_link_libraries)
With HEAD pointing at the previous commit, clang/lib/Analysis/plugins/CMakeLists.txt does not add the SampleAnalyzer subdirectory at all, because LLVM_ENABLE_PLUGINS has the value OFF. But CLANG_PLUGIN_SUPPORT is ON, so now it does, and the above error happens.
Jan 25 2022
Jan 24 2022
Jan 20 2022
Jan 12 2022
Jan 11 2022
Jan 5 2022
Jan 4 2022
Dec 3 2021
Ah, now I see what you mean – I didn't look far enough!
I don't think so, I'm afraid. If you look at the definition of MacroInfo::tokens_begin() in clang/include/clang/Lex/MacroInfo.h, you see that it doesn't return a raw Token *: it returns a C++ iterator object.
Nov 15 2021
Summarising what I said about argparse internally:
Improved the help text for the new variable.
I'm not sure it really makes sense to be able to override the output file location in the context of a build system – surely if you want to do that, you also have to tell cmake to change the OUTPUT parameter of the custom command, or the build products won't end up where the next build step expects them! Perhaps I should move this to after --mangling, though.
Nov 11 2021
Nov 10 2021
Oct 20 2021
In fact, it turns out that libxml2 optionally uses zlib – it's a configure-time option. And I've been able to work around this issue another way now on the platform where I had the problem, by recompiling the libxml2 static library using configure --without-zlib, which doesn't seem to have lost any of the libxml2 functionality that llvm-mt needs.
Oct 19 2021
Oct 15 2021
Sep 1 2021
Yes, that seems to have fixed it for me – thanks for the quick response!
Aug 31 2021
Jul 27 2021
Jul 22 2021
(But I've pushed this patch anyway, because I'm reasonably confident of the answer; if it turns out that there's some case along those lines that I should have taken care of, I'll fix or revert.)
@lattner, thanks for the help. In this case, the real question is whether there's any use case for !srcloc that involves writing it out into a bitcode or IR file and then having a separate instance of clang load it back in again.
Jul 21 2021
Jul 20 2021
Addressed two nits.
... and removed an unused function from the previous version.
Split up the allocations as suggested.
Renamed types to [U]IntTy. This will affect some of the dependent patches too, but I agree that saving two characters is worthwhile – these names are long enough already :-)
Added an i64 !srcloc to the only existing test of them I could find.
I looked into this yesterday, and realised that I don't actually know what the use case is for emitting !srcloc metadata in an IR file.
Jul 19 2021
Jul 12 2021
Jul 6 2021
This patch doesn't seem to have attracted much review attention in the last couple of weeks. On the theory that perhaps it's just too big and monolithic to review in one go, I've started to break it up into smaller pieces.
Jun 29 2021
Good catch!
Jun 24 2021
Hmmm. Two people have pointed out to me that my strategy of having a 32-bit SourceLocations::LowBits and an 0- or 32-bit SourceLocations::OptionalHighBits doesn't actually work, because an empty struct still takes at least 1 byte. So this version of the patch will still increase memory usage in the 32SL configuration, which is just what I was trying to avoid. Whoops.
Jun 18 2021
Thanks to @miyuki for repeating the previous benchmark with this version of the patch (and on the same machine as before, which was better than I could have done).
Jun 17 2021
@miyuki is working on other things at the moment, and I've picked this up.