lebedev.ri added inline comments.
================
Comment at: clang-doc/ClangDocBinary.h:82
+
+static std::map<BlockId, std::string> BlockIdNameMap = {
+ {NAMESPACE_BLOCK_ID, "NamespaceBlock"},
----------------
lebedev.ri wrote:
> Nice!
> Some thoughts:
> 1. I agree it makes sense to keep it close to the enum definition, in
> header...
> 2. This will result in global constructor. Generally they are frowned upon in
> LLVM. But since this is a standalone binary, it may be ok?
> 3. Have you tried using `StringRef` here, instead of `std::string`?
> 4. `std::map` is in general a bad idea.
> Since the `enum`'s enumerators are all small and consecutive, maybe try
> `llvm::IndexedMap`?
Also, this should be `static const`, since the underlying enum won't change on
the fly.
`#llvm` suggests to use TableGen here, i'm not sure how that would work.
As i have now noticed, there isn't a init-list constructor, so I think
**something** like this might work:
```
static const llvm::IndexedMap<BlockId> BlockIdNameMap = []() {
llvm::IndexedMap<BlockId> map;
map.reserve(BI_LAST);
// There is no init-list constructor for the IndexedMap, so have to improvise
static const std::initializer_list<std::pair<BlockId, const char* const>>
inits = {
{NAMESPACE_BLOCK_ID, "NamespaceBlock"},
...
};
for(const auto& init : inits)
map[init.first] = init.second;
}();
```
Also, even though `llvm::IndexedMap<>` is using `llvm::SmallVector<>`
internally, it does not expose the initial size as template parameter,
unfortunately, but hardcodes it to `0`. I think it would be great to add one
more template parameter to `llvm::IndexedMap<>`, which would default to `0`,
but would allow us here to avoid all memory allocation altogether.
What do you think? If you do agree that using `IndexedMap` seems like the right
choice, but **don't** want to write the patch for template parameter, i might
look into it..
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D41102
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
