jj10306 marked 29 inline comments as done.
jj10306 added inline comments.
================
Comment at: lldb/include/lldb/Target/TraceHTR.h:272
+ /// The map of block IDs to \a HTRBlock.
+ std::unordered_map<size_t, HTRBlock> const &GetBlockDefs() const;
+
----------------
jj10306 wrote:
> wallace wrote:
> > jj10306 wrote:
> > > wallace wrote:
> > > > As this is const, the only way to use this unordered_map is to do look
> > > > ups, then let's better hide the implementation detail that is an
> > > > unordered map and create this method instead
> > > >
> > > > const HTRBlock &GetBlockById(size_t id);
> > > >
> > > > that way we can later change unordered_map for anything else without
> > > > affecting any callers
> > > Makes sense. How should the case when an `id` that the layer doesn't
> > > contain is passed in? I would like to use `llvm::Optional` here but it's
> > > not possible to have an Optional reference (i.e. `Optional<& const
> > > HTRBlock>`). I was thinking about creating different methods:
> > >
> > > `bool ContainsBlock(size_t id) const` for checking if the layer contains
> > > an ID
> > > `HTRBlock const & GetBlockById(size_t id) const` for retrieving a
> > > reference to a block in the layer
> > > but this still doesn't solve the issue of if a caller calls `GetBlockId`
> > > with an invalid ID.
> > >
> > > Another option would be to change `GetBlockId` to return
> > > `Optional<HTRBlock>` instead of a reference, but this would result in an
> > > unnecessary copy, when all we really need is an const reference.
> > Optional<const HTRBlock &> should work like a charm. You can't put the &
> > before the const AFAIK. People normally read the & backwards as in
> > "reference to <const HTRBlock>"
> I tried what you suggested and also just a reference (no const), but it none
> of them work - this is the error message:
> ```
> llvm/include/llvm/ADT/Optional.h:281:5: error: 'getPointer' declared as a
> pointer to a reference of type 'lldb_
> private::HTRBlock &'
> T *getPointer() { return &Storage.getValue(); }
> ```
> A cursory search of the issue brought up many articles, here's one:
> https://www.fluentcpp.com/2018/10/05/pros-cons-optional-references/
Discussed offline - decided to return `HTRBlock * const` with a `nullptr`
indicating that the layer does not contain a bock with the specified block id.
================
Comment at: lldb/source/Target/TraceHTR.cpp:160
+ current_instruction_load_address);
+ valid_cursor = cursor.Next();
+ if (current_instruction_type &
----------------
wallace wrote:
> valid_cursor is not a nice name, just call it no_more_data, end_of_trace, or
> something like that
I was trying to use "positive logic", otherwise using something like
`end_of_trace` would require and negating every use of it and every time the
`Next()` is called (i.e `end_of_trace =!cursor->Next()`).
This is obviously a very minor thing, but I think using a well named "positive
logic" variable makes the code easier to read - thoughts on using a name like
`more_data_in_trace`? It's definitely harder to come up with a well named
positive logic variable so let me know if you have any better suggestions
================
Comment at: lldb/source/Target/TraceHTR.cpp:245
+ os.flush();
+ if (os.has_error()) {
+ s.Printf("error exporting trace representation to %s\n", outfile_cstr);
----------------
wallace wrote:
> can you also show the actual error message from os?
I don't believe `llvm::raw_fd_ostream` stores the error message, only the error
code.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D105741/new/
https://reviews.llvm.org/D105741
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
