[PATCH] D134055: [clang-doc] Export more enum information

[Paul Kirth via Phabricator via cfe-commits]

paulkirth added a comment.

Can you make the first line of the summary the commit title? It's a bit more descriptive.



================
Comment at: clang-tools-extra/clang-doc/BitcodeReader.cpp:53
 
+llvm::Error decodeRecord(const Record &R, llvm::APSInt &Field, llvm::StringRef Blob) {
+  auto ByteWidth = R[0];
----------------
do you need to do all of this? APSInt already supports to/from string methods, as well as converting to/from integers. can you use that here and in the writer to avoid some complexity?


================
Comment at: clang-tools-extra/clang-doc/BitcodeReader.cpp:221
     return decodeRecord(R, I->Loc, Blob);
-  case ENUM_MEMBER:
-    return decodeRecord(R, I->Members, Blob);
----------------
Don't you still need this? if it's now unused, we should probably remove it from the RecordId enum too.


================
Comment at: clang-tools-extra/clang-doc/Serialize.cpp:185
 
+SmallString<128> getSourceCode(const Decl* D, const SourceRange& R) {
+  return Lexer::getSourceText(
----------------
We normally try to avoid returning small string by value. see https://llvm.org/docs/ProgrammersManual.html#llvm-adt-smallstring-h

Normally in the case that we need to return a string from a method, we just use std::string. In other cases, it may be more appropriate to use an output parameter instead of a return.


================
Comment at: clang-tools-extra/clang-doc/Serialize.cpp:316
+  for (const EnumConstantDecl *E : D->enumerators()) {
+    SmallString<128> ValueExpr;
+    if (const Expr* InitExpr = E->getInitExpr())
----------------
why was 128 chosen? Aren't we storing it into a `SmallString<16>` in `EnumValueInfo`? is there some external reason that we expect this to be the right size? 

Do you have an idea for how long these are likely to be? if we expect them to be large or completely unpredictable, it may make more sense to use a `std::string` and avoid wasting stack space.

 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134055/new/

https://reviews.llvm.org/D134055