303506 - make xpcom nsDirectoryService assertions more meaningful

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Bob Clary [:bc] (inactive)]

When tracking assertions, simple asserts without caller information don't really
help in easily determining the location of the problem. While asserts do show the
file/line number, these can change over time making automatic historical
comparisons difficult.

Comment 1

[Bob Clary [:bc] (inactive)]

Attachment: patch

includes some tab cleanup.

Comment 2

[Doug Turner (:dougt)]

Comment on attachment 191664 [details] [diff] [review]
patch

when you split parameters on two different lines, please make sure that they
line up.  I think you missed the first one. 

To be helpful, next time, make an additional patch with the -w flag.  This will
allow everyone to see what the _real_ changes are without having to verify
every single ws change. 

This seams fine.  Maybe a better approach would make NS_NOTREACHED and other
fun macro use the predefined _FILE_ and _LINE_.  I think some compilers have a
_FUNCTION_ too... maybe that is the way we should fix all of these.  Bob, you
interested in doing that?

Comment 3

[Bob Clary [:bc] (inactive)]

(In reply to comment #2)
> (From update of attachment 191664 [details] [diff] [review] [edit])
> when you split parameters on two different lines, please make sure that they
> line up.  I think you missed the first one. 

It was already past col 80, to keep it from exceeding 80 col when splitting it
across the line, I had to indent it left by two cols. I could just leave the
line as it was originally.

> 
> To be helpful, next time, make an additional patch with the -w flag.  This will
> allow everyone to see what the _real_ changes are without having to verify
> every single ws change. 
> 

Sorry. 

> This seams fine.  Maybe a better approach would make NS_NOTREACHED and other
> fun macro use the predefined _FILE_ and _LINE_.  I think some compilers have a
> _FUNCTION_ too... maybe that is the way we should fix all of these.  Bob, you
> interested in doing that?
> 

I'm not sure I understand. It appears that NS_NOTREACHED already uses
nsDebug::Assertion and passes __FILE__ and __LINE__ as do the macros that call
nsDebug::Warning. I don't have much (any) cross-platform experience, but I can
look into it and let you know what I find.

__FILE__ and __LINE__ are fine as far as they go, but when the source is
modified, the "signature" of the assertion or warning changes. With an
"pseudo-unique" identifiable string as the message, I can strip the file and
line out and look for changes in assertions without false positives due to line
migration.

Comment 4

[Doug Turner (:dougt)]

I would leave lines like that alone.  

No worrys about the -w diff... just a suggestion for the future.

Comment 5

[Bob Clary [:bc] (inactive)]

Regarding using __FUNCTION__, it does not appear to be easily used. It is not
defined in VC6 although it does appear in VC7. It is defined in GCC, but its
interpretation varies depending on GCC version and whether the program is C or
C++. In C++, __FUNCTION__ only returns the method name not including the Class
name. In addition, nsIDebug is "under review". 

Comment 6

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 191664 [details] [diff] [review]
patch

sr=shaver

Comment 7

[Bob Clary [:bc] (inactive)]

Doug, could you go ahead and check this in? I've failed in my attempts to check
in two other patches tonight. I guess I only have restricted checkin rights.

Comment 8

[Bob Clary [:bc] (inactive)]

I found my cvs horkage. Will check this in to trunk later today.

Comment 9

[Bob Clary [:bc] (inactive)]

Checking in xpcom/io/nsDirectoryService.cpp;
/cvsroot/mozilla/xpcom/io/nsDirectoryService.cpp,v  <--  nsDirectoryService.cpp
new revision: 1.84; previous revision: 1.83