Remove the ifdefs for pr_logging from storage code

RESOLVED FIXED in mozilla28

Status

()

enhancement
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: ally, Assigned: weeska)

Tracking

unspecified
mozilla28
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=mak][lang=c++][qa-])

Attachments

(1 attachment, 4 obsolete attachments)

Storage has lots of #ifdef PR_LOGGING, which then calls PR_LOG, so doesn't need the macro.  There are also a bunch of DEBUG ones, which are safe without the ifdef because the macros will be a no op if they are not defined.
(In reply to comment #0)
> Storage has lots of #ifdef PR_LOGGING, which then calls PR_LOG, so doesn't need
> the macro.  There are also a bunch of DEBUG ones, which are safe without the
> ifdef because the macros will be a no op if they are not defined.
Note that some of the #ifdefs are there because we do extra work and then warn or log.  However, if we don't do extra work before then, there is no reason to wrap it in the #ifdefs
Whiteboard: good first bug
Ally, do you still think this is a good first bug? If so, maybe you could describe the problem a little bit more, or add yourself as a mentor.  I'm following up on older bugs that are marked "good first" and that haven't been changed in a long time. Thanks!
Flags: needinfo?(ally)
I think it might be. That said, I have not looked at that code in ..2+ years, and I am not a reviewer for storage, so I am not qualified to mentor this bug. I imagine :mak would have a more informed opinion.
Flags: needinfo?(ally) → needinfo?(mak77)
yes, there are still points where it's pointless to wrap PR_LOG with #ifdef PR_LOGGING or #ifdef DEBUG, because PR_LOG is a no-op in builds without PR_LOGGING.
It may still be useful where there is additional code needed by the PR_LOG invocation, as comment 1 said.
Flags: needinfo?(mak77)
Whiteboard: good first bug → [good-first-bug][mentor=mak][lang=cpp]
Hi Marco,

I am interested in fixing this. Kindly let me know the complete details. Thanks a lot!
Hi, I am interested in working on this bug. So, I request you to assign it to me and guide me on how to get started with because I am a newbie. I am trying to fix the PR_LOG error and if you assign it to me I will complete the patch work for this.

Thanks in advance,
A.Anup
Assignee: nobody → allamsetty.anup
Hi Anup, here's some basic information on how to submit a patch. 

https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch
Marco Bonardo:

Sir,

I have created a patch file and attached it for a review. I have a small question, if we want to test this particular bug how to proceed on it sir?

Thanks in Advance,

Regards,
A.Anup
In the following attachment I have added a file by name "diff.txt" in which all the changes to the code are saved. Hope I will get a review as soon as possible. 

Thanks in Advance,

Regards,
A.Anup.
Attachment #788036 - Attachment is obsolete: true
Attachment #788070 - Flags: ui-review?(josh)
Attachment #788070 - Flags: superreview?(mak77)
Attachment #788070 - Flags: review?(mak77)
Attachment #788070 - Flags: review?(lhenry)
Attachment #788070 - Flags: review?(josh)
Attachment #788070 - Flags: review?(ally)
Attachment #788070 - Flags: feedback?(josh)
Attachment #788070 - Flags: feedback?(ally)
Attachment #788070 - Flags: feedback
Comment on attachment 788070 [details] [diff] [review]
In this I have made a few changes to the code and uploaded.

This is not what was intended, unfortunately. When we talk about removing the #ifdef, we really mean removing the entire #ifdef line, and the corresponding #endif. This won't compile as written. Furthermore, this is removing #ifdefs that are unrelated to PR_LOG, which is outside the scope of the changes talked about.
Attachment #788070 - Flags: ui-review?(josh)
Attachment #788070 - Flags: superreview?(mak77)
Attachment #788070 - Flags: review?(mak77)
Attachment #788070 - Flags: review?(lhenry)
Attachment #788070 - Flags: review?(josh)
Attachment #788070 - Flags: review?(ally)
Attachment #788070 - Flags: feedback?(josh)
Attachment #788070 - Flags: feedback?(ally)
Attachment #788070 - Flags: feedback
Make sure to always build Firefox again before submitting a patch; that's a good way to ensure that you're not going to break anything, regardless as to whether you know how to test the changes or not.
Attachment #788070 - Attachment is obsolete: true
Attachment #788228 - Flags: review?(josh)
Attachment #788228 - Flags: review?(lhenry)
Attachment #788228 - Flags: review?(lhenry) → review?
Hi Anup, as we discussed on irc, you should just flag the mentor for the bug, who is mak.  You may not get a reply immediately. I understand you are doing this for a school assignment, but these things do take time. Please be patient!
Attachment #788228 - Flags: review?(mak77)
Attachment #788228 - Flags: review?(josh)
Attachment #788228 - Flags: review?
Anup, please don't request checkin on patches that haven't been reviewed yet.
Keywords: checkin-needed
It's been a very long time that is why I have tried for a checkin. Sorry for that.
It's been a very long time I submitted a patch, that is why I have tried for a checkin. 

Sorry for that
Comment on attachment 788655 [details] [diff] [review]
contains some more modifications comparing to the first patch.

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

Apologize for the long delay, I assume this is the most recent patch to review.
When attaching newer version of the patches, please obsolete the previous ones.

I went through and commented each single case, but mostly they are similare or the same.
The basic rule here is that we fix only call points that don't do additional work apart from calling a macro, as well explained by comment 1.
So basically, this is doing a bit too much.

::: storage/public/mozIStorageValueArray.idl
@@ +82,1 @@
>      nsresult rv =

this should be changed to a DebugOnly<nsresult> otherwise it would warn an unused variable

The same should be done for all of the other changes similar to this one (so when a variable definition is the only thing inside an ifdef DEBUG). So, I'm not going to repeat the comment, but is valid for any of those cases.

::: storage/src/VacuumManager.cpp
@@ +76,3 @@
>    warnMsg.AppendLiteral(" ");
>    warnMsg.Append(message);
>    NS_WARNING(warnMsg.get());

this should not be changed, since in non debug mode we don't need to build a warnMsg

@@ +240,5 @@
> +    warnMsg.AppendLiteral(" - ");
> +    warnMsg.AppendInt(result);
> +    warnMsg.AppendLiteral(" ");
> +    warnMsg.Append(message);
> +    NS_WARNING(warnMsg.get());

same here, please revert as it was before

As a general rule, any work that is needed only in debug mode should be ifdefed.

@@ +256,4 @@
>             ("Vacuum failed with error: %d '%s'. Database was: '%s'",
>              result, message.get(), mDBFilename.get()));
>    }
> +

This piece of code is doing extra work apart the PR_LOG, so the ifdef should not be removed, see comment 1

::: storage/src/mozStorageAsyncStatement.cpp
@@ +145,3 @@
>    // We want to try and test for LIKE and that consumers are using
>    // escapeStringForLIKE instead of just trusting user input.  The idea to
>    // check to see if they are binding a parameter after like instead of just

this piece of code is doing more than just warn, so it should stay ifdef debug as before

@@ +293,5 @@
>    // Make sure we are never called on the connection's owning thread.
>    bool onOpenedThread = false;
>    (void)mDBConnection->threadOpenedOn->IsOnCurrentThread(&onOpenedThread);
>    NS_ASSERTION(!onOpenedThread,
>                 "We should only be called on the async thread!");

doing more work, should stay ifdef

::: storage/src/mozStorageAsyncStatementExecution.cpp
@@ +227,5 @@
>    mMutex.AssertNotCurrentThreadOwns();
>  
>    bool onCallingThread = false;
>    (void)mCallingThread->IsOnCurrentThread(&onCallingThread);
>    NS_ASSERTION(onCallingThread, "runEvent not running on the calling thread!");

should stay ifdef

@@ +322,2 @@
>    // Check to make sure that this statement was smart about what it did.
>    checkAndLogStatementPerformance(aStatement);

should stay ifdef

@@ +545,3 @@
>    bool onCallingThread = false;
>    (void)mCallingThread->IsOnCurrentThread(&onCallingThread);
>    NS_ASSERTION(onCallingThread, "Not canceling from the calling thread!");

should stay ifdef

::: storage/src/mozStorageAsyncStatementJSHelper.cpp
@@ +36,4 @@
>    int32_t state;
>    (void)aStatement->GetState(&state);
>    NS_ASSERTION(state == mozIStorageAsyncStatement::MOZ_STORAGE_STATEMENT_READY,
>                 "Invalid state to get the params object - all calls will fail!");

should stay ifdef

@@ +102,5 @@
>    {
>      nsISupports *supp = aWrapper->Native();
>      nsCOMPtr<mozIStorageAsyncStatement> isStatement(do_QueryInterface(supp));
>      NS_ASSERTION(isStatement, "How is this not an async statement?!");
>    }

should stay ifdef

::: storage/src/mozStorageConnection.cpp
@@ +353,5 @@
>      if (!onCallingThread) {
> +
> +      bool onAsyncThread = false;
> +      (void)mAsyncExecutionThread->IsOnCurrentThread(&onAsyncThread);
> +      MOZ_ASSERT(onAsyncThread);

should stay ifdef

@@ +636,2 @@
>    if (!gStorageLog)
>      gStorageLog = ::PR_NewLogModule("mozStorage");

should stay ifdef

@@ +811,2 @@
>    // Sanity checks to make sure we are in the proper state before calling this.
>    NS_ASSERTION(mDBConn, "Database connection is already null!");

should stay ifdef

@@ +829,5 @@
>    nsAutoCString leafName(":memory");
>    if (mDatabaseFile)
>        (void)mDatabaseFile->GetNativeLeafName(leafName);
>    PR_LOG(gStorageLog, PR_LOG_NOTICE, ("Closing connection to '%s'",
>                                        leafName.get()));

should stay ifdef

@@ +842,3 @@
>      NS_WARNING(msg);
>      ::PR_smprintf_free(msg);
>    }

should stay ifdef

::: storage/src/mozStoragePrivateHelpers.cpp
@@ +75,5 @@
>    nsAutoCString message;
>    message.AppendLiteral("SQLite returned error code ");
>    message.AppendInt(rc);
>    message.AppendLiteral(" , Storage will convert it to NS_ERROR_FAILURE");
>    NS_WARNING(message.get());

should stay ifdef

::: storage/src/mozStorageStatement.cpp
@@ +173,1 @@
>    // We want to try and test for LIKE and that consumers are using

should stay ifdef

@@ +505,4 @@
>    PR_LOG(gStorageLog, PR_LOG_DEBUG, ("Resetting statement: '%s'",
>                                       ::sqlite3_sql(mDBStatement)));
>  
>    checkAndLogStatementPerformance(mDBStatement);

only the checkAndLog should still be ifdef

@@ +583,5 @@
>        nsAutoCString errStr;
>        (void)mDBConnection->GetLastErrorString(errStr);
>        PR_LOG(gStorageLog, PR_LOG_DEBUG,
>               ("Statement::ExecuteStep error: %s", errStr.get()));
>    }

doing more work so should stay ifdef

::: storage/src/mozStorageStatementData.h
@@ +88,5 @@
> +    // method would return nullptr as a result.
> +    if (asyncThread) {
> +      bool onAsyncThread;
> +      NS_ASSERTION(NS_SUCCEEDED(asyncThread->IsOnCurrentThread(&onAsyncThread)) && onAsyncThread,
> +                   "This should only be running on the async thread!");

should stay ifdef

::: storage/src/mozStorageStatementJSHelper.cpp
@@ +51,5 @@
>        do_QueryInterface(wrapper->Native())
>      );
>      NS_ASSERTION(isStatement, "How is this not a statement?!");
>    }
> +

every changes in this file should be reverted
Comment on attachment 788228 [details] [diff] [review]
Consists of the changes made as per the details in the bug.

obsoleted by new version, that I just reviewed.
Attachment #788228 - Attachment is obsolete: true
Attachment #788228 - Flags: review?(mak77)
Whiteboard: [good-first-bug][mentor=mak][lang=cpp] → [good-first-bug][mentor=mak][lang=c++]
Assignee: allamsetty.anup → nobody
Whiteboard: [good-first-bug][mentor=mak][lang=c++] → [good first bug][mentor=mak][lang=c++]
I just looked if i'd work on this ticket, but it seems it is already completed. Should this be closed?
The patch needs to be modified according to the requests in comment 19. It might be easier to start from scratch; I'm not sure.
Attachment #826406 - Flags: review+
Attachment #826406 - Flags: review?(mak77)
In future you want to set the review flag to ?, and mark old patches as obsolete.
Thanks, i'll do that.
Comment on attachment 826406 [details] [diff] [review]
remove ifdefs for pr_logging where they're not needed

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

This patch looks good, though it's missing a piece compared to the previous one, but I'm fine to have those handled in a separate patch:
- in mozIStorageValueArray.idl most of the "#ifdef DEBUG\nnsresult rv =\n#endif\n" can be replaced by DebugOnly<nsresult> rv =
Attachment #826406 - Flags: review?(mak77) → review+
Assignee: nobody → weeska
Keywords: checkin-needed
Attachment #788655 - Attachment is obsolete: true
> - in mozIStorageValueArray.idl most of the "#ifdef DEBUG\nnsresult rv
> =\n#endif\n" can be replaced by DebugOnly<nsresult> rv =

I've splitted this part to bug 939297, so this can be resolved on landing, feel free to take the new bug if you wish. Thanks.
OS: Windows 7 → All
Hardware: x86 → All
Summary: Remove the ifdefs for debug and pr_logging from storage code → Remove the ifdefs for pr_logging from storage code
https://hg.mozilla.org/mozilla-central/rev/444714c3820a
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [good first bug][mentor=mak][lang=c++] → [good first bug][mentor=mak][lang=c++][qa-]
You need to log in before you can comment on or make changes to this bug.