> 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;
> }
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)
- 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
