This patch is a refactor to implement prepend afterwards. Since this changes a lot of files and to conform with guidelines, I will separate this from the implementation of prepend. Related to the discussion in https://reviews.llvm.org/D111414 , so please read it for more context.
Details
- Reviewers
dblaikie ldionne - Group Reviewers
Restricted Project - Commits
- rG2e97236aacbb: [Demangle] Rename OutputStream to OutputString
Diff Detail
Event Timeline
llvm/include/llvm/Demangle/ItaniumDemangle.h | ||
---|---|---|
15 | Why those changes ? IIUC the CI is failing on this file, too. |
llvm/include/llvm/Demangle/ItaniumDemangle.h | ||
---|---|---|
15 | This is because the script was verbatim copied. I think LLVM should address this in a more clean way, to be honest. These specific files are literally copied from one place to another and stuff like this can happen without being noticed. @dblaikie Currently, there is an effort to change this or any specific rationale to keep it this way? |
llvm/include/llvm/Demangle/Utility.h | ||
---|---|---|
12–14 | Another header guard that shouldn't be changed? |
Include guards are now the same as before.
llvm/include/llvm/Demangle/Utility.h | ||
---|---|---|
12–14 | Sorry, I missed that file too. Should be good now. |
llvm/include/llvm/Demangle/ItaniumDemangle.h | ||
---|---|---|
15 | Not quite following - but no, I don't think there's any effort at the moment to change this situation that these two files need to be kept in sync. |
libcxxabi/src/demangle/Utility.h | ||
---|---|---|
27 | This was arguably written with the intent of being a stream -- that's why the class provides functions like operator<<. On the other hand, it also provides some functions like operator+=, which don't really belong in a stream, but do belong in a string. I feel like this patch as-is kind of breaks what this class was supposed to be, since now we'll have a type that is neither a string nor a stream, but has both operator+= and operator<<. Do we use operator<< widely? If not, would it make sense to remove it altogether and make this closer to a real string? Otherwise, perhaps we could consider naming this something like OutputBuffer instead, which IMO makes it less vexing to have an operator<< defined on. I don't want to stall this solely on picking a name, but I do feel like having a class called OutputString yet boasting operator<< is really un-idiomatic and I'd like to explore alternatives. |
libcxxabi/src/demangle/Utility.h | ||
---|---|---|
27 | OutputBuffer? |
libcxxabi/src/demangle/Utility.h | ||
---|---|---|
27 | Yeah, that was my suggestion above. I'm still not a big fan of it but I find it a bit less weird than OutputString given the existence of operator<<. |
I amended the diff. I also renamed variable names to match the new type name, otherwise OS and S names would not make much sense. I renamed them to be OB.
llvm/include/llvm/Demangle/ItaniumDemangle.h | ||
---|---|---|
15 | Those files need to be manually copied and manually changed to prevent the usage of the same include guard. This can be improved by e.g. generate a copy of this file with a generator script on the build system. I asked if there is any rationale to have it this way. |
llvm/include/llvm/Demangle/ItaniumDemangle.h | ||
---|---|---|
15 | Still not sure I'm following/understanding the question. If you mean: Is there any reason this is manual rather than automated? I doubt it, only that no one's managed to automate it yet, perhaps. |
clang-format: please reformat the code