jj10306 marked an inline comment 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;
+
----------------
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/
================
Comment at: lldb/include/lldb/Target/TraceHTR.h:339
+ /// The file that the exported HTR data will be written to.
+ void Export(Stream &s, TraceExportFormat export_format, std::string outfile);
+
----------------
wallace wrote:
> jj10306 wrote:
> > wallace wrote:
> > > better return an llvm::Error that will contain the error message, if any.
> > > The CommandObject that invokes this method can then get the error and
> > > dump it if necessary. So, don't pass any streams here
> > I based this method off of `TraceCursor::DumpInstructions` which takes in a
> > Stream & to output its success/error messages to. This method
> > (`TraceHTR::Export`) prints a message to the console on success (not only
> > failure) so I'm not sure how the success message could be propagated back
> > up to the caller. I agree that using `llvm::Error` here would make sense if
> > we were only printing if an Error occurred, but since we print on success
> > I'm not sure if we should use llvm::Error over passing the Stream.
> It's different, because DumpInstructions is supposed to dump information in
> the stream, but in this case Export is printing to a file. So what if someone
> wants to invoke Export but not from a command? e.g. from a python script. You
> are making this case a little bit more complicated.
> You can just return llvm::success() and let the caller print anything to
> Stream if available. In any case, it's standard in lldb to not print anything
> if the operation is a success
Ah I see, that makes sense. I'll switch it to use `llvm::Error` and not print
anything in the case of success, because the success indication isn't
particularly useful.
================
Comment at: lldb/source/Target/TraceHTR.cpp:439-440
+
+ // Can't use ConstString here because no toJSON implementation exists for
+ // ConstString
+ llvm::Optional<std::string> most_freq_func =
----------------
wallace wrote:
> jj10306 wrote:
> > wallace wrote:
> > > this comment doens't make sense, as you are instead passing display_name
> > > to the json object
> > I was trying to indicate that `GetMostFrequentlyCalledFunction` returns a
> > `std::string` instead of `ConstString` because eventually the `ConsString`
> > function name will need to be converted to `std::string` in order to be
> > serialized, but I will remove this comment and just change the function to
> > return `ConstString` and then do the conversion in the ternary that defines
> > `display_name`.
> you shouldn't change a generic API just to ease a single instance of a usage.
> Better return a ConstString, or even better, a llvm::StringRef, which can
> point to anything. ConstString has a method to get a StringRef out of it.
> StringRef::str() gets you a std::string if you need it, or you can use
> StringRef::data() to get the const char *, which helps you avoid one copy.
Will update that function to return a `StringRef` 🙂
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
