[PATCH] D134055: [clang-doc] Add support for explicitly typed enums

[Brett Wilson via Phabricator via cfe-commits]

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:
> brettw wrote:
> > 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.
> Those are fair points, and I think I misread/misunderstood a bit of what's going on here.
> 
> As for encoding/decoding integers,  BitcodeWriter already has integer support, so if you need to convert to/from those, it should already work, right?
> 
> Regardless, you may want to look at the bitcode reader/writer in llvm to see how they serialize/deserialize APInt, as their implementation seems a bit more straightforward IMO.
> 
> https://github.com/llvm/llvm-project/blob/9050a59c6678789831b7286b8b68d18b966c4694/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp#L1636
> https://github.com/llvm/llvm-project/blob/main/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp#L2520
> https://github.com/llvm/llvm-project/blob/9050a59c6678789831b7286b8b68d18b966c4694/llvm/lib/Bitcode/Reader/BitcodeReader.cpp#L2843
> https://github.com/llvm/llvm-project/blob/9050a59c6678789831b7286b8b68d18b966c4694/llvm/lib/Bitcode/Reader/BitcodeReader.cpp#L2743
I just converted everything to a string which allowed most of this code to be deleted. I don't think any theoretical benefit of keeping the APSInt is worth the complexity.


================
Comment at: clang-tools-extra/clang-doc/BitcodeReader.cpp:71
+
+  llvm::SmallVector<uint64_t, 1024> AsWords;
+  AsWords.resize(WordWidth);
----------------
paulkirth wrote:
> You can avoid the resize w/ SmallVector(Size) constructor, right?
This is now deleted.


================
Comment at: clang-tools-extra/clang-doc/Representation.h:425
+  // constant. This will be empty for implicit enumeration values.
+  std::string ValueExpr;
+};
----------------
paulkirth wrote:
> Sorry to nitpick, but SmallString was the correct choice to use in this type. We avoid its use as a return value, because it tends to be brittle, and stops us from assigning into arbitrary sized SmallStrings.  In the struct, it's a reasonable choice, especially if you expect most uses to be small.  for these expressions, I expect most to either be the number itself, or some shift operation, so SmallString<16> was probably more than sufficient.
In the common case there will be will be empty, so std::string actually seems more efficient. Also, I personally think the current quantity of SmallString in this code is over-optimization.


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

https://reviews.llvm.org/D134055