History.cpp:931:19: warning: unused variable 'navHistory' [-Wunused-variable] (and another similar unused variable further down)

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Blocks 1 bug)

Trunk
mozilla34
All
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Linux opt build warning:
{
/toolkit/components/places/History.cpp: In constructor 'mozilla::places::{anonymous}::InsertVisitedURIs::InsertVisitedURIs(mozIStorageConnection*, nsTArray<mozilla::places::VisitData>&, mozIVisitInfoCallback*)':
/toolkit/components/places/History.cpp:931:19: warning: unused variable 'navHistory' [-Wunused-variable]

/toolkit/components/places/History.cpp: In member function 'virtual nsresult mozilla::places::History::GetPlacesInfo(JS::Handle<JS::Value>, mozIVisitInfoCallback*, JSContext*)':
/toolkit/components/places/History.cpp:2695:17: warning: unused variable 'navHistory' [-Wunused-variable]
}
https://tbpl.mozilla.org/php/getParsedLog.php?id=45321945&tree=Try

These are both just unused variables that should be dropped. (They're only "used" in debug builds only because we have a null-check assertion.)

For the record, the first variable's only usage was removed here (for bug 561450), leaving it unused:
https://hg.mozilla.org/mozilla-central/diff/69f4d3ba11a5/toolkit/components/places/History.cpp#l1.96

...and the second variable was never used in the first place. It was added with no usages here (for bug 834539):
http://hg.mozilla.org/mozilla-central/diff/734147446def/toolkit/components/places/History.cpp#l1.910
Blocks: buildwarning
(In reply to Daniel Holbert [:dholbert] from comment #0)
> https://tbpl.mozilla.org/php/getParsedLog.php?id=45321945&tree=Try

(That Try log shows these build warnings treated as errors, since it had a patch in the queue to add FAIL_ON_WARNINGS to this directory.)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #8468613 - Flags: review?(mano)
Attachment #8468613 - Attachment description: fix → fix: drop the unused variables
(mak: if you happen to notice this before mano, feel free to steal the review. Trivial patch; just removing two unused variables.)
Comment on attachment 8468613 [details] [diff] [review]
fix: drop the unused variables

Review of attachment 8468613 [details] [diff] [review]:
-----------------------------------------------------------------

I'm pretty sure those exist because we need to be sure history service is up and running before proceeding.
I think you rather want DebugOnly here, MOZ_ASSERT, and add a comment.
Attachment #8468613 - Flags: review?(mano) → review-
(In reply to Marco Bonardo [:mak] (Away 15-31 Aug) from comment #4)
> I'm pretty sure those exist because we need to be sure history service is up
> and running before proceeding.

I don't think I agree.

I can interpret your comment in two ways:
 (1) We need to check if GetHistoryService() fails, and bail out if it does because we can't usefully proceed.

If this is what you mean, I disagree that this is useful, because we don't actually bail out on failure right now (we just assert, in debug builds).  So we're not usefully reacting to  GetHistoryService() failures here.  So I don't see any reason to call it in opt builds; it's just useless overhead.

OR, if your meaning was:
 (2) We need to call GetHistoryService() to make sure some state is set up for something to use later.

If this is what you mean, I think I disagree with this, too. GetHistoryService() seems to return a lazy-initialized variable. So if something is going to use it later, then we can do the lazy-initialization when that later thing tries to get it.  No need to prematurely force the lazy-initialization.

Am I missing something?
Flags: needinfo?(mak77)
(In reply to Daniel Holbert [:dholbert] from comment #5)
> I can interpret your comment in two ways:
>  (1) We need to check if GetHistoryService() fails, and bail out if it does
> because we can't usefully proceed.
> 
> If this is what you mean, I disagree that this is useful, because we don't
> actually bail out on failure right now (we just assert, in debug builds). 
> So we're not usefully reacting to  GetHistoryService() failures here.  So I
> don't see any reason to call it in opt builds; it's just useless overhead.

OK, so the fact is we will need history service later, but at that time we are on another thread so we cannot initialize it.
I agree we might want to handle errors better, even if this is very unlikely to be hit.

In GetPlacesInfo let's null check history and return error.
In InsertVisitedURIs we are in a constructor so we can't return an error. Let's move the check into InsertVisitedURIs::Start and return an error from there.

Does this work better for you?
Flags: needinfo?(mak77)
(In reply to Marco Bonardo [:mak] (Away 15-31 Aug) from comment #6)
> OK, so the fact is we will need history service later, but at that time we
> are on another thread so we cannot initialize it.

Ah, this makes more sense. Thanks for clarifying.

> In GetPlacesInfo let's null check history and return error.
> In InsertVisitedURIs we are in a constructor so we can't return an error.
> Let's move the check into InsertVisitedURIs::Start and return an error from
> there.
> 
> Does this work better for you?

I think so -- I'll give that a shot. Thanks!
Patch updated per comment 6.
Attachment #8468613 - Attachment is obsolete: true
Attachment #8470102 - Flags: review?(mak77)
Comment on attachment 8470102 [details] [diff] [review]
fix v2: bail if navHistory is null

Review of attachment 8470102 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you
Attachment #8470102 - Flags: review?(mak77) → review+
https://hg.mozilla.org/mozilla-central/rev/5709d54523ef
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.