brettw added inline comments.
================
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];
----------------
paulkirth wrote:
> 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?
I don't think converting to an integer is a good idea because people sometimes
use min/max values for enum values and since this could be signed or unsigned,
it gets kind of complicated.
Serializing a number as a string to bitcode also seemed wrong to me.
The simplest thing would be to store the value as a string in the EnumValueInfo
and then always treat this as a string from then on. If you want it simplified,
I think that's the thing to do. But I thought you would want the numeric value
stored in clang-doc's "ast" because some backends may want to treat this as a
number, check its signed/unsignedness, etc. I happy to change this if you want.
================
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);
----------------
paulkirth wrote:
> Don't you still need this? if it's now unused, we should probably remove it
> from the RecordId enum too.
I think this is only used for properties and not the child members, so it isn't
used. I removed the enum.
================
Comment at: clang-tools-extra/clang-doc/Serialize.cpp:185
+SmallString<128> getSourceCode(const Decl* D, const SourceRange& R) {
+ return Lexer::getSourceText(
----------------
paulkirth wrote:
> 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.
I was just copying some other code in this file, I changed this to std::string.
================
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())
----------------
paulkirth wrote:
> 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.
>
>
I changed this and the EnumValueInfo struct to std::string. I think the usage
of SmallString in these records is over the top but I was trying to copy the
existing style.
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
