[PATCH] D42765: [DebugInfo] Support DWARFv5 source code embedding extension

Jonas Devlieghere via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Feb 2 01:26:08 PST 2018


JDevlieghere added inline comments.


================
Comment at: include/llvm/IR/DIBuilder.h:151
+                       StringRef Checksum = StringRef(),
+                       Optional<StringRef> Source = None);
 
----------------
scott.linder wrote:
> JDevlieghere wrote:
> > I'm not a fan of these two things not being consistent. Either both should be options or both should be empty StringRefs? This seems to be recurring throughout this patch. I think `Optional` better conveys the idea, but would require more API changes. 
> I agree, but would like feedback on the approach to fixing it.
> 
> The new code uses an `Optional` to explicitly support an empty source string, without it being assumed to simply be absent, so that `None != ""`. While it may not have many practical uses, it seems better to be explicit.
> 
> The old code is also explicit, but the paired `ChecksumKind` is what encodes whether a checksum is available. I do not want to duplicate this, as it leads to inconsistencies when `CSKind != CSK_None`, but `Checksum == None`, or vice-versa.
> 
> The best option I see is to wrap the pair in an optional (`Optional<pair<CSKind, Checksum>>`), remove the `CSK_None` variant, and maybe make the pair an explicit struct with members for the kind and checksum. Would this be an acceptable fix?
I like the idea of the explicit struct (`DIFile::Checksum`?) wrapped in an optional. If it's not too big of a change we might as well move the `ChecksumKind ` enum in there too?


Repository:
  rL LLVM

https://reviews.llvm.org/D42765





More information about the llvm-commits mailing list