> On Oct. 1, 2014, 11: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?
Steve may have been bikeshedding, but I was doing was probably closer to
http://xkcd.com/386/ :)
- Nathan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/2442/#review5400
-----------------------------------------------------------
On Sept. 29, 2014, 10: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, 10: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
