return rv?rv:NS_ERROR_FAILURE; looks ugly

RESOLVED FIXED

Status

()

RESOLVED FIXED
17 years ago
17 years ago

People

(Reporter: bernd_mozilla, Assigned: bernd_mozilla)

Tracking

Trunk
x86
Windows 98
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Assignee)

Description

17 years ago
at 
http://lxr.mozilla.org/seamonkey/source/layout/html/table/src/nsTableRowGroupFrame.cpp#1734
the following code snippet can be found:

if(NS_FAILED(rv) || !tempParentFrame)
 return rv?rv:NS_ERROR_FAILURE;

this should look like:

if (NS_FAILED(rv))
  return rv;
if (!tempParentFrame)
  return NS_ERROR_FAILURE;

a search at lxr for rv?rv reveals more than 20 hits. I will attach a patch to
bring this search down to zero.
(Assignee)

Comment 1

17 years ago
sorry Marc, this should be my work
Assignee: attinasi → bernd.mielke
(Assignee)

Comment 2

17 years ago
Created attachment 52380 [details] [diff] [review]
the layout part of the work
Comment on attachment 52380 [details] [diff] [review]
the layout part of the work

r=dbaron, although I would prefer if you put the return statements on a
separate line, e.g.
if (NS_FAILED(rv))
  return rv;
rather than
if (NS_FAILED(rv)) return rv;
mainly since that makes it easier to work with in a debugger.
Attachment #52380 - Flags: review+
(Assignee)

Comment 4

17 years ago
Created attachment 52948 [details] [diff] [review]
patch with Davids comments
Comment on attachment 52948 [details] [diff] [review]
patch with Davids comments

Except for the missing ')' on the line below, r=dbaron.
>Index: html/base/src/nsTextFrame.cpp
>+    if (NS_FAILED(rv)
Attachment #52948 - Flags: review+

Comment 6

17 years ago
Comment on attachment 52948 [details] [diff] [review]
patch with Davids comments

Make it compile, and sr=attinasi 

+    if (NS_FAILED(rv)
Attachment #52948 - Flags: superreview+
(Assignee)

Comment 7

17 years ago
Created attachment 53605 [details] [diff] [review]
the editor part

Comment 8

17 years ago
Comment on attachment 53605 [details] [diff] [review]
the editor part

There are a couple of places where you made this change:


-  if (NS_FAILED(rv) || !aContent)
-    return rv?rv:NS_ERROR_FAILURE;
-  else
+  if (NS_FAILED(rv))
+    return rv;
+  if (!aContent)
+    return NS_ERROR_FAILURE;
+  
     return NS_OK;


Make sure you outdent the "return NS_OK;" that used to be under the "else".

I probably would've made it look more like ...


-  if (NS_FAILED(rv) || !aContent)
-    return rv?rv:NS_ERROR_FAILURE;
-  else
-    return NS_OK;
+  if (!aContent)
+    return NS_ERROR_FAILURE;
+  
+  return rv;



... though that's not a requirement for checkin.


Thanks for removing this pattern. It's been the source of several bugs in the past!

sr=kin@netscape.com
Attachment #53605 - Flags: superreview+
(Assignee)

Comment 9

17 years ago
The NS_OK is correctly indented in my source, I posted a diff -uw which may be
confusing.

Comment 10

17 years ago
Comment on attachment 53605 [details] [diff] [review]
the editor part

Note that editor/base is supposed to be removed, so those changes don't need to
be checked in.
All editor files are in the libeditor and composer directories.
Attachment #53605 - Flags: review+

Comment 11

17 years ago
r=cmanske
(Assignee)

Comment 12

17 years ago
Created attachment 54677 [details] [diff] [review]
get rid of the remaining occurences

Comment 13

17 years ago
Comment on attachment 54677 [details] [diff] [review]
get rid of the remaining occurences

sr=attinasi
Attachment #54677 - Flags: superreview+

Comment 14

17 years ago
Comment on attachment 54677 [details] [diff] [review]
get rid of the remaining occurences

r=kin@netscape.com
Attachment #54677 - Flags: review+

Comment 15

17 years ago
Comment on attachment 54677 [details] [diff] [review]
get rid of the remaining occurences

r=kin@netscape.com
(Assignee)

Comment 16

17 years ago
fix checked in
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.