> On Oct. 1, 2014, 4:37 p.m., Steve Reinhardt wrote:
> > Are the bool cast operators being used anywhere? It looks like you put
> > them in, but then do the 'ptr != nullptr' thing everywhere we need a bool,
> > so I'm wondering if they're useful.
> >
> > How about replacing the cast operator with 'bool isNull() const { return p
> > == nullptr; }', then all of the checks can just be 'ptr.isNull()'?
>
> Andreas Hansson wrote:
> There are some 50+ occurrences where the bool operator is used. I'd
> prefer to leave it as it if that's ok.
>
> Nathan Binkert wrote:
> Steve, I'm surprised that you'd suggest getting rid of an operator
> bool(), why? I thought you hated the != nullptr or == nullptr kind of checks.
>
> Nathan Binkert wrote:
> FYI, the use of a variable inside an if () is considered explicit. As is
> the use in conjunction with logical operators. The only place where it is
> implicit is if you were doing something like:
>
> bool foo() {
> return ptr;
> }
>
> Steve Reinhardt wrote:
> I'm fine with leaving the bool operator in. The only reason I said
> 'replacing' rather than 'augmenting' in my initial comment is because I
> didn't see where the bool conversion was used in this patch, and didn't see
> any explicit casts in the existing code. Thinking more broadly, I do see a
> few uses of '!p', and one use of txPacket in a boolean expression, so it is
> getting used... so yes, let's keep it. I don't see anywhere near 50 uses
> though; just out of curiosity, what am I missing?
>
> You're right that my gut reaction was really opposition to the 'ptr !=
> nullptr' checks and not against the bool operator itself. Looking more
> closely at the cases where we do need the explicit conversion, I see that
> it's really just to generate a flag for serialization; probably overkill to
> make a method just for that. I think it's more readable if you add some
> parens though, e.g.:
> bool packet_exists = (packet != nullptr);
>
> Another option would be to rewrite these cases, which look like this:
> bool txPktExists = txPacket;
> SERIALIZE_SCALAR(txPktExists);
> if (txPktExists)
> along these lines:
> SERIALIZE_SCALAR(bool(txPacket));
> if (txPacket)
>
>
> Nathan Binkert wrote:
> Also, with operator bool, we shouldn't need operator!
>
> you can't do that with SERIALIZE_SCALAR because it is a macro and we
> stringify the parameter. We should probably have a version of the macro that
> allows for a cast though because that idiom does happen frequently.
>
> And finally,
>
> bool foo = bar; // implicit cast
> bool foo{bar}; // explicit cast
> auto foo = bool{bar}; // equivalent explicit cast (and argued as
> preferred by Sutter
> http://herbsutter.com/2013/08/12/gotw-94-solution-aaa-style-almost-always-auto/)
>
> Andreas Hansson wrote:
> I'm keen to file this under bikeshedding :-)
>
> Is there a take-away in terms of changes to this patch or is it good to
> go?
>
> Nathan Binkert wrote:
> Steve may have been bikeshedding, but I was doing was probably closer to
> http://xkcd.com/386/ :)
>
Sorry, I got headed down the wrong path because I was focusing solely on the
code in the patch, and didn't realize there were existing uses of operator
bool() even though it was newly added. (And thus the explicit nullptr
comparisons that were in the patch are the exception rather than the common use
case.)
Just for readability, I'd prefer to see the nullptr comparisons parenthesized,
as I mentioned above, but other than that this looks fine.
- Steve
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/2442/#review5400
-----------------------------------------------------------
On Sept. 29, 2014, 3:39 a.m., Andreas Hansson wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/2442/
> -----------------------------------------------------------
>
> (Updated Sept. 29, 2014, 3:39 a.m.)
>
>
> Review request for Default.
>
>
> Repository: gem5
>
>
> Description
> -------
>
> Changeset 10438:290510e58445
> ---------------------------
> dev: Use shared_ptr for EthPacketData
>
> This patch transitions the EthPacketData from the ad-hoc
> RefCountingPtr to the c++11 shared_ptr. There are no changes in
> behaviour, and the code modifications are mainly replacing "new" with
> "make_shared".
>
> The bool casting operator for the shared_ptr is explicit, and we must
> therefore either cast it, compare it to NULL (p != nullptr), double
> negate it (!!p) or do a (p ? true : false).
>
>
> Diffs
> -----
>
> src/base/inet.hh 28b31101d9e6
> src/dev/etherlink.cc 28b31101d9e6
> src/dev/etherpkt.hh 28b31101d9e6
> src/dev/ethertap.cc 28b31101d9e6
> src/dev/i8254xGBe.cc 28b31101d9e6
> src/dev/ns_gige.cc 28b31101d9e6
> src/dev/pktfifo.cc 28b31101d9e6
> src/dev/sinic.cc 28b31101d9e6
>
> Diff: http://reviews.gem5.org/r/2442/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andreas Hansson
>
>
_______________________________________________
gem5-dev mailing list
[email protected]
http://m5sim.org/mailman/listinfo/gem5-dev
