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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
