832673 - Preprocessor.py should give an error if there's junk (e.g. an attempt to write a comment) on an #ifdef (etc) line

Status: NEW

(Firefox Build System :: General, defect)

Description

[Jonathan Kew [:jfkthame]]

When adding a rather large block of platform-specific prefs to all.js, I "helpfully" included comments on the #ifdef ... #else ... #endif lines I was adding. (As it's a JS file, I naturally enough wrote //-style comments, without a second thought.) See attachment 704225 [details] [diff] [review] in bug 831354 for an example.

However, this caused Preprocessor.py to fail to match the intended #ifdef symbol (presumably it included the entire comment in the symbol it was testing, or something like that).

It would be far more helpful to terminate the build with an error if there's spurious, unparseable junk on the preprocessor lines, if it can't be made to "correctly" recognize and ignore comments (given that it may not know what comment syntax to expect, depending on the intended language of the file).

Comment 1

[Mike Hommey [:glandium]]

Something like that, I guess:

--- a/config/Preprocessor.py
+++ b/config/Preprocessor.py
@@ -274,17 +274,18 @@ class Preprocessor:
       self.ifStates[-1] = self.disableLevel
     else:
       self.ifStates.append(self.disableLevel)
     pass
   def do_ifdef(self, args, replace=False):
     if self.disableLevel and not replace:
       self.disableLevel += 1
       return
-    if re.match('\W', args, re.U):
+    args = args.strip()
+    if not re.match('\w+$', args, re.U):
       raise Preprocessor.Error(self, 'INVALID_VAR', args)
     if args not in self.context:
       self.disableLevel = 1
     if replace:
       if args in self.context:
         self.disableLevel = 0
       self.ifStates[-1] = self.disableLevel
     else:

Comment 2

[Mike Hommey [:glandium]]

For good measure, we should error out on #else and #endif too.