75853 - CR and LF are naked ifdefs

Status: VERIFIED FIXED

(SeaMonkey :: Build Config, defect)

Description

[Mike Pinkerton (not reading bugmail)]

in nsCRT.h, |CR| and |LF| are naked ifdefs. this means that no one is allowed to 
have members, fields, or variable names called these. Normally, this wouldn't be 
a problem, but there are structs in mac headers in OSX that use CR (it's a 
powerPC register name). As a result, this blocks OSX work.

so off i go to make a nice patch to change every occurrance in the tree. just try 
and stop me.

Comment 1

[Mike Pinkerton (not reading bugmail)]

anyone have a good perl script to do this? ;)

Comment 2

[timeless]

Attachment: slightly overzealous

Comment 3

[Brendan Eich [:brendan]]

RCS file:
/cvsroot/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_asm_ppc_aix.s,v
retrieving revision 1.3
diff -u -0 -r1.3 xptcinvoke_asm_ppc_aix.s
--- xpcom/reflect/xptcall/src/md/unix/xptcinvoke_asm_ppc_aix.s  2000/01/19
01:19:26     1.3
+++ xpcom/reflect/xptcall/src/md/unix/xptcinvoke_asm_ppc_aix.s  2001/04/13
06:09:01
@@ -39 +39 @@
-.set CR0_EQ,2
+.set NS_CR0_EQ,2

Slightly overzealous?  Try flat wrong.  Use a keyword grep (grep -w, \b on each
side in a perl regexp, \< and \> in vi/ex, etc.).

/be

Comment 4

[Mike Shaver (:shaver emeritus)]

Why are we using another ifdef here?  Aren't we just begging to have to do this
dance again when some _other_ loserly platform gets broken?

Let's do nsCRT::CR and nsCRT::LF.

Comment 5

[timeless]

Attachment: irrelevant files removed. some comments still changed, should I change comments?

Comment 6

[timeless]

ok, so we want nsCRT::LF, nsCRT::CR ? that should be easy to do (i'll just 
modify the current patch and add the stuff to nsCRT)

Comment 7

[Chris Waterson]

Isn't the problem with nsCRT::LF cases where someone has done this:

  const char* myTwoLineString = "line one" CR "line two"

Not sure if that case exists in reality, or if it does, whether we should fix
the usage rather than accomodate it.

Comment 8

[Simon Fraser [no longer active]]

nsCRT::CR will have to be an enum, right?

Comment 9

[timeless]

yes, waterson's comment is the unfortunate answer to shaver's question.  
However the lines remaining in my patch don't indicate that usage.
I think that internationalization pretty much forced all real display strings 
out of c++ files so the only uses left should be in parsing.

i have a tree that uses the class version, however, i'm having problems getting 
a non patched build to run so if someone else could help me test the patch, 
that'd be good.

Comment 10

[Brendan Eich [:brendan]]

New patch coming up?

/be

Comment 11

[timeless]

ok. simon suggested a namespace enum, and i decided to try that approach.  This 
patch is currently building on my system, but it did result in a few minor 
changes because of overloaded functions (foo(int a) foo(char a)). i'm going to 
attach a diff -u0 patch because my changes do not affect code flow.

The enum supports more than just the two chars currently in question, but in 
order to do this I ellected to move all of the defines to the end of the code.  
the char defines should be killed by me later when I search for and convert any 
uses. the string defines can live either at the top or bottom, i'll let 
reviewers, module owners, or file owners dictate.

Comment 12

[timeless]

Attachment: namespace patch

Comment 13

[Simon Fraser [no longer active]]

This won't work:

+namespace NSCRT {
+  enum {
+    TAB='\011' /* \t Horizontal Tab */,
+    LF /* \n Line Feed */,
+    VTAB /*  Vertical Tab */,
+    FF /*    Form Feed */,
+    CR /* \r Carriage Return */
+  };
+}

You have to assign each with the correct value. And why not just put the enum 
inside nsCRT, rather than making the ugly NSCRT?

Comment 14

[timeless]

sorry, the compilers i'm used to (borland, and aparently microsoft) have the 
following behavior (quoting)

Syntax
enum [<type_tag>] {<constant_name> [= <value>], ...} [var_list];
	<value> must be an integer. If <value> is missing, it is assumed to be:
<prev> + 1

where <prev> is the value of the previous integer constant in the list. For the 
first integer constant in the list, the default value is 0.

You said namespace, perhaps i misinterpretted, i can and will move into nsCRT.

are there any more Comments about my interpretation of enum?

Comment 15

[Simon Fraser [no longer active]]

Never mind, I missed that the order of items in the enum makes them consecutive.
Even though they are, however, I'd recommend that you assign them explicitly, so 
we can see what the individual values are without counting.

As an side, why do people always use Octal for these things? Why not hex? Who 
thinks in octal these days?

Comment 16

[Mike Pinkerton (not reading bugmail)]

i would prefer the constants being explicit.

Comment 17

[Brendan Eich [:brendan]]

Don't use namespaces (see
http://www.mozilla.org/hacking/portable-cpp.html#dont_use_namespace).

Do give each enumerator an explicit value.  Anything else for (small or not)
enums whose enumerators are not obviously, necessarily dense, is poor style.

/be

Comment 18

[timeless]

Attachment: class, fixed enum (not sure about the values)

Comment 19

[Brendan Eich [:brendan]]

Given all the files you have to touch, why not do away with constants and use
the manifest '\r', '\n', etc. fine backslash-escaped character constants?  They
are part of C and C++, have been forever.

/be

Comment 20

[Simon Fraser [no longer active]]

@@ -99,0 +88,7 @@
+  enum {
+    TAB='\t'  /* Horizontal Tab */,
+    LF='\n'   /* Line Feed */,
+    VTAB='\v' /* Vertical Tab */,
+    FF='\f'   /* Form Feed */,
+    CR='\r'   /* Carriage Return */
+  };

iirc, there are sometimes compiler settings that case \r and \n to be reversed 
(e.g. MPW newline mode in CodeWarrior), but we need to guarantee that these enums 
are always the correct values. So I'd change this enum to use 0x09...0x0D.

Comment 21

[Brendan Eich [:brendan]]

sfraser: do we use MPW newline mode in CW?  I ask because the JS engine, which
has been widely ported, uses '\r' and '\n' to mean what they say.

/be

Comment 22

[Simon Fraser [no longer active]]

No, we don't use MPW newline mode in general. However, if someone was building 
parts of mozilla as MPW tools (maybe xpidl, xptlink, JS?) then they would have to 
turn this option on. I'm not sure how much of an issue this is.

Comment 23

[timeless]

yeah as i was away from my computer i realized that just using \r \n seemed to 
make more sense.

Pinkerton: you filed this bug, would converting everything inline to use \n 
form be ok w/ you?

would using some numberical \ form be ok for the things which might be compiled 
as MPW tools?

i'm told the attached patch isn't applying correctly, so i'll work on another 
one in an hour

Comment 24

[timeless]

Attachment: "final" enum patch.

Comment 25

[timeless]

ok, if anyone wants to build MPW all they'd need to do is make a new nsCRT.h 
file or add an ifdef to it to handle that special case. It's not really my 
problem, however if someone is concerned they can feel free to file a new bug 
on me to support that (requires detailed documentation about what it means 
etc).  This patch is ready for review, approval and hopefully checkin.

Comment 26

[Scott Collins]

prefer |char(nsCRT::LF)| over |(char)nsCRT::LF|, etc.  After you fix that, sr=scc.

Comment 27

[Daniel Veditz [:dveditz]]

r=dveditz

Comment 28

[timeless]

fix checked in. someone will have to clean up the commercial side
perl -p -e 's/\bCR\b/nsCRT::CR/g;s/\bLF\b/nsCRT::LF/g' <filelist>

and [requires a bit more baby sitting]:
perl -p -e '
s/(Append.*)nsCRT::CR/$1char(nsCRT::CR)/g
s/(Append.*)nsCRT::LF/$1char(nsCRT::LF)/g
' <filelist>

Comment 29

[Mike Pinkerton (not reading bugmail)]

i could care less how we fix it, just get CR out of the global namespace.

Comment 30

[Jon Granrose]

has anyone filed a bug yet to do this to the ns tree?

Comment 31

[timeless]

No one complained about problems in the NS tree. I think there might 
be one or two places in comm w/ their own definitions, but i don't know if they 
spill into global land.

pinkerton: verifyme.

Comment 32

[Jon Granrose]

verified. have any Netsape engineers checked the ns tree for this yet?