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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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

> The idea for error handling for DataExtractor sounds reasonable, looks like adding an error flag wouldn't even increase the size.


Hmm... Originally I was thinking of building something on top of DataExtractor. Putting the logic *into* the DataExtractor is an interesting idea. I kind of like it (it would solve the problem I had of how to capture the DataExtractor vs. DWARFDataExtractor relationship in the "on top" model), but there's also something that bothers me about that. I think it's the fact that this would make the DataExtractor class stateful, whereas previously it was completely stateless. That may not me all bad, but it would mean the transition has to be done more carefully (watch out for thread races, and other unintended effects of the error bit leaking out). However, it also feels weird to have the error flag be a part of DataExtractor state, while the offset isn't. So, e.g. if one extracts from the DataExtractor using two independent offsets simultaneously, the error state set by one extraction would impact the other. This would be most obvious with the strtab-style data extractors, which almost always get a bunch of completely independent queries.

One way to achieve this while keeping the DataExtractor stateless would be to pass the error flag as an additional argument to the extraction methods, just like the offset is now. But that would make things more verbose, which means one might still want to implement some kind of an abstraction on top of that to keep these things together...


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63591