Closed Bug 1485142 Opened Last year Closed Last year

LookupCache.h:63:12 [-Wreturn-std-move] local variable 'hex' will be copied despite being returned by name

Categories

(Toolkit :: Safe Browsing, enhancement, P5)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Build warning with clang trunk (version 8.0):
=====
toolkit/components/url-classifier/LookupCache.h:63:12 [-Wreturn-std-move] local variable 'hex' will be copied despite being returned by name
=====

This is for this chunk of code:

>  nsCString PartialHashHex() const {
>    nsAutoCString hex;
>    for (size_t i = 0; i < mPartialHashLength; i++) {
>      hex.AppendPrintf("%.2X", hash.complete.buf[i]);
>    }
>    return hex;
>  }

Right now, "return hex" makes us convert from nsAutoCString into an actual nsCString, which probably means doing heap allocation to store the string that we've built up in the local stack "auto" buffer.

This is silly. Let's just make this function return nsAutoCString directly, so that it can benefit from return value optimization, and so that its |hex| local variable can live a bit longer and get reused without copying in the calling function.

(As it happens, this function is only called 3 times in some logging code, like so:
>    LOG(("Confirmed result %s from table %s",
>         result->PartialHashHex().get(), result->mTableName.get()));
...so there's no reason that we need this function to return a nsCString -- we just call get() on its return value once, log that value, and then we're done.)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Priority: -- → P5
Before this patch -- with the nsCString return type -- we have to do heap
allocation and copying to produce the return value.  But the callers don't
actually care about having a nsCString -- they just call .get() to access the
character buffer, and log it, and then they're done. They can do this just as
easily with the actual local nsAutoCString that PartialHashHex() works with
locally, so let's change the return type so that return value optimization
can give them that variable directly and avoid needless copying/allocation.

This patch addresses the following clang 8.0 build warning:
 LookupCache.h:63:12 [-Wreturn-std-move]
 local variable 'hex' will be copied despite being returned by name
Comment on attachment 9002913 [details]
Bug 1485142: Make url-classifier 'PartialHashHex()' API return a nsAutoCString instead of nsCString, to address build warning & reduce copying. r=gcp

Gian-Carlo Pascutto [:gcp] has approved the revision.
Attachment #9002913 - Flags: review+
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/73e2097aa0c3
Make url-classifier 'PartialHashHex()' API return a nsAutoCString instead of nsCString, to address build warning & reduce copying. r=gcp
https://hg.mozilla.org/mozilla-central/rev/73e2097aa0c3
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.