103495 - xpidl does not allow a space beteen # and include in XPCOM IDL

Status: RESOLVED INCOMPLETE

(Core :: XPCOM, defect, P5)

Description

[Michael Klepikov]

IDL below not compile with diagnostics:
bug.idl:1: `nsISupports' undeclared identifier

# include "nsISupports.idl"
[uuid(BA419180-388C-11d3-A5B3-00C04F8EEB79)]
interface nsIFoo : nsISupports {};

If I remove the space between # and include, it works fine.

Comment 1

[Michael Klepikov]

Attachment: Sample IDL file to reproduce the problem

Comment 2

[John Bandhauer]

FWIW... if it were up to me I'd mark this as WONTFIX and suggest to people 
that they might not want to leave any spaces between the '#' and the 'include'.

Comment 3

[John Bandhauer]

mike72: Is the tool's intolerance of those spaces a hardship for you? or just a 
surprise?

Comment 4

[Michael Klepikov]

It does not create hardship, that's why I filed it as "minor". But it does not
let me write how I usually write, and it's surprising to see a construct that
looks so similar to familiar C preprocessor but still has a subtle difference in
syntax (I know there is no preprocessor in XPCOM IDL). I am not writing part of
Mozilla code so I don't think I should be forced to follow Mozilla-specific
cosmetic guidelines.

Also the diagnostics is confusing: it complains well past the place that made it
not well formed XPCOM IDL, i.e. the space after #. One has to look carefully at
the diagnostics to realize that the line number pertains to #include and not to
the inheritance spec of one of the interfaces. I spent some time carefully
checking the spelling of nsISupports and whether my interface declaration syntax
was correct.

It's ok if it's not fixed -- it's only a nuisance once you hit it the first
time. I just thought it would be easy to fix -- just consider # and include as
separate tokens.

Comment 5

[John Bandhauer]

Fair enough. I'll certainly leave it up to dbradley to determine where this fits 
in his view of priorities. Of course, the presence of a debugged patch would 
likely have an impact on priorities :)

This does look easier to fix than I imagined at first since it looks like fixing 
this would only impact xpidl_idl.c and would apparently not require hacking 
libIDL (which is outside our codebase). Still, this is somewhat brittle code and 
there are plenty of other ways that it is unfreindly to 'incorrect' input. Given 
the plethora of other things worth fixing I'd leave it pretty low on the list 
if it were up to me. I *do* appreciate that you went to the trouble to report 
it. Sorry if I came off otherwise.

Comment 6

[Phil Schwartau]

Confirming for condideration - 

Comment 7

[Phil Schwartau]

Um, "consideration" !

Comment 8

[David Bradley]

Interesting, I would have bet money this was handled by at least the lexer. The
code as it stands today appears to be quite brittle. I suspect adding more than
one space between #include and the quote will cause this to wig out as well.
This would have been automatically handled had it been parsed within the lexer.

I went ahead and cooked up patch. I'll attach it in a bit, once I've confirmed
it works under Linux.

Comment 9

[David Bradley]

Attachment: Allows spaces between #, include, and the first quote mark

Comment 10

[David Bradley]

Comment on attachment 52557 [details] [diff] [review]
Allows spaces between #, include, and the first quote mark

Forgot to account for spaces before #

Comment 11

[David Bradley]

Attachment: Same as previous patch, but allows for spaces before #

Comment 12

[David Bradley]

Check the edge cases. I missed about handling spaces before #. I don't think
there are any others. Also this allows for #include"file.h", which I think
should be allowed as well, but wasn't. Basically this adds logic that would
normally have been handled by the lexer, probably not as robust, but good enough.

Comment 13

[John Bandhauer]

FWIW, space before the '#' is not supported by some C preprocessors for 
platforms we support  - or used to support - (please don't ask which ones).

Comment 14

[David Bradley]

I thought I had encountered that long ago in some compiler. I had checked the
generated header and the include's are reformatted.

The only issue would be if we ever plan on using a C preprocessor to preprocess
the IDL files. I know the author of libIDL originally wanted to do this. But
access to a C preprocessor wasn't universal across platforms, IIRC. I think Mac
is one such platform.

Comment 15

[John Bandhauer]

FWIW. I only mentioned that about C preprocessors as a data point to suggest 
that fixing leading spaces is of lesser importance - since bahavior more like C 
preprocessors was the stated goal. Looking for a '#' in column zero wouuld be 
acceptable in my book.

Comment 16

[Johnny Stenback (:jst)]

If SkipWhiteSpace() needs to worry about '\n', should it then not also worry
about '\r'? Or do we need to care about newlines at all here?

Comment 17

[David Bradley]

I may have erroneously assumed that the xpidl file was opened as a text file and
thus any \r\n would get translated to \n. Other than completeness I don't think
there is any reason to check for line endings. And isspace would seem more
appropriate. Don't know why I didn't use that in the first place.

Comment 18

[David Bradley]

Attachment: Patch with isspace

Comment 19

[Johnny Stenback (:jst)]

Comment on attachment 55906 [details] [diff] [review]
Patch with isspace

r=jst

Comment 20

[David Bradley]

Moving out

Comment 21

[David Bradley]

After further discussion it was decided 1.0.1 makes more sense as a post 1.0
milestone.

Comment 22

[David Bradley]

Retargetting to the proper post 1.0 milestone

Comment 23

[David Bradley]

Moving out to 1.3. If this needs to be in before 1.3 please comment.

Comment 24

[David Bradley]

Moving to 1.4 Alpha

Comment 25

[David Bradley]

Moving out

Comment 26

[David Bradley]

Moving out

Comment 27

[David Bradley]

Moving out