- User Since
- May 26 2014, 12:49 PM (220 w, 6 d)
Fri, Aug 17
Is it possible for
I suspected something would be wrong with that approach, it would be too
simple otherwise :) lgtm
Thu, Aug 16
Can’t we just use a base class with pure virtual methods, pass it to the
demangle function, implement this in clang, and pass null from libcxxabi?
Moving the whole thing to a header file seems kinda unfortunate.
But do you actually need those changes that pavel is working on? That looks
like a whole bunch of code that someone who just wants to print a demangled
name won’t care about. How involved are bugs usually, because I’m imagining
they’re a) usually just a couple lines change and b) pretty infrequent.
Once the relicense happens, you just change the include path and include
the real one.
IIRC it’s ?A0xABCDABCD@ where the hex value is some kind of hash
Another option is to simply move it into support and say that going forward
merges to libcxxabi might get more difficult. Perhaps this isn’t so bad
though. Isn’t the itanium demangler mostly complete? If the potential for
future changes to it is low, maybe this is an acceptable tradeoff. Then we
could just move it to support, use it and evolve it any way we want with no
restrictions, and when new functionality gets added to the itanium mangler
that needs to be port to libcxxabi, it’s a little more effort than a simple
copy paste because you have to think about it some.
If the licensing issue is that libc++abi can't include code from Support, then it also can't include Compiler.h. So I feel like this code move is just a bandaid instead of a proper solution. Can we use dependency injection or something to get some hooks into the demangler that allow you to customize the behavior from Clang / LLVM, as described in D50828?
If we're not going to allow code from Support to be used in the demangler, then I don't think we should move it to support, because it will be too easy for people to mess up and use code they shouldn't.
Thanks for being patient. Looking forward to actually using this on my Linux box.
I had a couple of other comments, but since I responded from email since I was on the go and I guess they didn't show up inline. Sorry about that. If you prefer I can resubmit them all as inline comments, or I guess you can just respond to the email thread.
Wed, Aug 15
Looking pretty good, I went over it again and found a few more things. There's a bit more auto than I'm comfortable with, but I'm not gonna make a big deal about it. it does make the code a bit hard to read when it's used for trivial return values though.
This looks great. I've tried to reduce the MSVC debug build time as well,
but I didn't find this particular problem. I can't comment on the
correctness since I don't know much about how tablegen is implemented, but
I just wanted to say that this kind of thing is very appreciated and I hope
you continue doing more of it :)
Tue, Aug 14
Mon, Aug 13
I would split this into 3 separate patches.
Sorry for the delay, was on vacation last week.
Ok I’ll take a look later today then when i get in
Fri, Aug 10
Thu, Aug 9
Wed, Aug 8
lgtm, although Predicate is simple enough that I wonder if one day we should try to just delete it entirely.
To elaborate, if you install the C/C++ plugin, and you go to Debug ->
Configurations, and you go down to the MICmdMode property, it is by default
set to "gdb". But you can change this to "lldb" and it works out of the
How does this differ from VS Code's builtin LLDB MI support?
Tue, Aug 7
No that’s fine, only makes sense to use it if it fits naturally. As long as
you tried. Lgtm as is
We have a function somewhere in lit utils that does this, but I’m OOO so I
can’t look for it. Can you try to find it?
Mon, Aug 6
Ok 60kb is probably fine. Lgtm
If you go to vs settings you can go to project -> build & run -> output
level and set it to Diagnostic. Then Ctrl+F for LLVMInstallDir and you
should see the value. Try this with @ and with \ and see if they’re
In that case I think the first version was correct. LLVM@LLVM means “the
value LLVM under the key LLVM”. LLVM\LLVM means “the default value under
the key LLVM which itself is under the key LLVM”
Did you see my comments on the first round about how the CMake build didn’t
work? Because I don’t see any changes to CMakeLists.txt here, which means
it still won’t work.
I’m OOO this week so I can’t easily check this stuff. Sorry for all the
questions. The registry key that we create, is it the (Default) value or is
it a string value named LLVM?
I am OOO this week and only have access to mobile, so hopefully someone
else can review it
I’m pretty sure I tested this code path, was it wrong? Also I don’t think
we should be explicitly adding the \ after the $(LLVMInstallDir), if you
look through the way every other path in a VS project works, they assume
the \ is in the registry path, so if it’s possible I think we should follow
the same pattern.
Sun, Aug 5
std::function might be able to construct itself efficiently from a lambda,
but once it’s in the std::function all that info is lost and moving it
around more will always copy (unless you explicitly move). We only really
assign it once, so this isn’t an issue, but unique_function is nice from a
purist point of view in that it won’t ever allow std::function’s slow path
to be taken, even accidentally.
Sat, Aug 4
Fri, Aug 3
Looks like i was wrong, nevermind!
I think we have llvm::contains() which would allow you to make this
slightly better. Other than that, good find!
For the Register stuff, for example, I think it could make sense for it to be in a project such as HAL (Hardware Abstraction Layer) or something similarly named. Everything that describes properties of specific CPUs could go there, perhaps even including ArchSpec. For the Scalar stuff, perhaps a target called Visualization (or even just Value). Once more and more stuff starts ending up in Utility, I think it makes sense to try something like this.
Several months ago when this came up, i hypothesized that Utility would
eventually contain a mix of random stuff some of which may not make sense
in the long term. But that in the meantime, it’s useful to have some sort
of layering abstraction for “has no other dependencies”. Eventually when
this reaches critical mass, a natural separation should present itself,
because all the stuff that conceptually belongs together will be in
Utility. So then we can break Utility up into a more logical separation
No objections here
Thu, Aug 2
Should the substitution just be surrounded by quotes automatically so we
don’t have this problem?
Please remember to test with the cmake build when you add or remove files,
as that is the build that all of the buildbots use. I almost reverted this
since it broke every LLDB buildbot, but I noticed that it's just forgetting
to remove the files from the CMakeLists.txt so I'll fix it.
Wed, Aug 1
Use boolean variables instead of a Flags enum.
Tue, Jul 31
It would be an extra line of code, but I think this would be clearer (with
even less chance of collision) if you just choose a random number in [0,35]
and map it to [0-9][a-z]. The bit twiddling and array indexing seems like
unnecessary cleverness and someone could accidentally come along and break
it if they weren’t careful.
How could 16 characters not be enough? That’s 35^16 unique file names?