shafik added a comment.

In D58792#1415390 <https://reviews.llvm.org/D58792#1415390>, @clayborg wrote:

> In D58792#1414964 <https://reviews.llvm.org/D58792#1414964>, @labath wrote:
>
> > In D58792#1414191 <https://reviews.llvm.org/D58792#1414191>, @shafik wrote:
> >
> > > It stood out to me that some of the conversions were not `const` and I 
> > > can see that `IsValid` is not consistently `const` across the API but 
> > > after talking to @jingham it is unfortunately something we can't change.
> >
> >
> > Yes, that is unfortunate. I can think of three things that we could do 
> > differently though:
> >
> > 1. add a `const` version of `IsValid` where it is missing, and have and 
> > always-const `operator bool` which uses that
> > 2. give up on constness and just have a non-const `operator bool` everywhere
> > 3. add a const `operator bool` everywhere, and have `IsValid` (const or 
> > non-const) call into that
>
>
> "const" is useless when your class contains a shared pointer or a unique 
> pointer. It also changes the API mangling which we can't do. So I vote for 
> give up on const as you can call non const methods in any shared and unique 
> pointers because const is only protecting the pointer from changing.
>
> > Each option has different tradeoffs, and it's not really clear to me which 
> > one is better. I am happy to implement whichever you think is best.


The goal of `const` is to communicate to the user that it does not mutate the 
underlying object, which in general is good practice to maintain if possible.

In this case I don't see any good trade-offs here. I don't feel like adding a 
`const` version along w/ a non-`const` version really helps since it still 
muddies the waters.


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

https://reviews.llvm.org/D58792



_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to