[Lldb-commits] [PATCH] D63591: DWARFDebugLoc: Make parsing and error reporting more robust

Fri Jun 21 07:17:49 PDT 2019

labath added a comment.

In D63591#1553600 <https://reviews.llvm.org/D63591#1553600>, @probinson wrote:

> Ah, hadn't considered statefulness.  But if you layer another class on top of DataExtractor to handle the error flag, it would have to be replicating all the offset-is-valid checks, because of course DataExtractor itself doesn't return errors.

Not really. The data extractor kind of does return errors, as we've seen in this patch, it's just that they're incredibly hard to check for. I was thinking of having the new class rely on the SavedOffset == Offset behavior, but only internally. In my prototype, I managed to tuck it all away into a single template function which takes a member function pointer argument :D. Unfortunately, that was ICE-ing on gcc :P, but that was because I was beeing too clever -- I'm sure it can be dumbed down a bit.

> I have a couple more ideas to toss out there...
> 
> - A DataExtractorBase class that returns Optional<whatever>, and then DataExtractor layers on top and converts None to zero, which preserves the non-statefulness as well as the current API. This adds some runtime overhead, not sure how much.

That would work. It wouldn't even have to be a separate class, if you just make sure the function names are somehow different. However, it would mean that one has to explicitly check the result of every `get` operation. Not the end of the world, but it would make the code using it more noisy.

> - Or, a template DataExtractorBase that takes an error-handling class as a parameter (sort of like how STL containers take an allocator) and a DataExtractor specialization uses a no-op error-handling class. Should avoid runtime overhead at the cost of template cruft.

I'm not exactly sure how you imagined that, but I'm sure it could be made to work, as templates can be made to do almost anything. :P I'm not sure if it would be simpler than having a wrapper class or not. I have a feeling it might end up looking fairly similar from the outside.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D63591