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;

  // 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..

  rCTE Clang Tools Extra


cfe-commits mailing list

Reply via email to