Some SVG header files are missing #ifndef/#define/#endif wrapping

RESOLVED FIXED

Status

()

Core
SVG
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: dholbert, Assigned: Felipe Corrêa da Silva Sanches)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

8 years ago
Just noticed that some .h files in content/svg/content/src/ are missing the conventional "#ifndef/#define/#endif" wrapping.

In particular, grep says that these files lack any mention of "ifndef":
  nsSVGAElement.h
  nsSVGElementList.h
  nsSVGFeaturesList.h
  nsSVGMpathElement.h
  nsSVGPolyElement.h
(Assignee)

Comment 1

8 years ago
Created attachment 447234 [details] [diff] [review]
added the header wrappings
Attachment #447234 - Flags: review?(dholbert)
Assignee: nobody → felipe.sanches
(Reporter)

Comment 2

8 years ago
Comment on attachment 447234 [details] [diff] [review]
added the header wrappings

>--- a/content/svg/content/src/nsSVGElementList.h	Mon May 24 14:48:24 2010 +0200
>+++ b/content/svg/content/src/nsSVGElementList.h	Mon May 24 22:00:21 2010 +0200
>@@ -62,6 +62,9 @@
>  *   A convenience value indicating support for none of the above
>  */
> 
>+#ifndef NS_SVGAELEMENT_H_
>+#define NS_SVGAELEMENT_H_

Copy-paste error -- this should be NS_SVGELEMENTLIST_H_. :) Looks good aside from that! r=dholbert with the above fixed.

Requesting additional review from jwatt, even though this is a trivial-ish change, just to be on the safe side.
Attachment #447234 - Flags: review?(jwatt)
Attachment #447234 - Flags: review?(dholbert)
Attachment #447234 - Flags: review+
(Reporter)

Comment 3

8 years ago
(In reply to comment #0)
> In particular, grep says that these files lack any mention of "ifndef":
> [...]
>   nsSVGMpathElement.h

Note that this patch doesn't touch nsSVGMpathElement.h, and that's fine, because it's already got the correct wrapping.  I was mistaken to list it in comment 0. (It didn't even exist in m-c until 2 weeks after comment 0.  The copy caught by grep in comment 0 was a WIP version in my local tree. :))
(Reporter)

Updated

8 years ago
Status: NEW → ASSIGNED
Comment on attachment 447234 [details] [diff] [review]
added the header wrappings

Thanks for checking. nsSVGElementList.h and nsSVGFeaturesList.h should not have include guards. Can you attach a new patch with your changes to nsSVGAElement.h and nsSVGPolyElement.h, and with a comment added to the top of nsSVGElementList.h and nsSVGFeaturesList.h to say "no include guard" please Filipe?
Attachment #447234 - Flags: review?(jwatt) → review-
(Assignee)

Comment 5

8 years ago
Created attachment 447301 [details] [diff] [review]
Fixes the header wrappings and adds "no include guards" message to nsSVGElementList.h and nsSVGFeaturesList.h

Sorry about the copy/paste mistake. 

I am curious to know what is the reason for not having include guards around nsSVGElementList.h and nsSVGFeaturesList.h
Attachment #447234 - Attachment is obsolete: true
Attachment #447301 - Flags: review?(jwatt)
Comment on attachment 447301 [details] [diff] [review]
Fixes the header wrappings and adds "no include guards" message to nsSVGElementList.h and nsSVGFeaturesList.h

Perfect, thanks Filipe! Do you have commit access to be able to push this to mozilla-central yourself?
Attachment #447301 - Flags: review?(jwatt) → review+
Sorry, Felipe, not Filipe.

The reason is because these are not normal headers with declarations that must only be declared once. The macro inside those headers has to be is defined by the file that includes the header, to do whatever repeated thing is required for all the items listed in the header. Although these headers are not currently used twice in the same file, it should be possible to use them like that. There are other headers like these in the Mozilla codebase that are like this that are included multiple times into the same file, but I can't think of examples offhand.
(Assignee)

Comment 8

8 years ago
(In reply to comment #6)
> (From update of attachment 447301 [details] [diff] [review])
> Perfect, thanks Filipe! Do you have commit access to be able to push this to
> mozilla-central yourself?

Not yet. What is the commit access policy?
http://www.mozilla.org/hacking/committer/
(Assignee)

Comment 10

8 years ago
Thanks, Robert. I am reading it now. But I think that I still don't really need such access.

Can somebody else push this code into mozilla-central, please?
In general you should go to the keywords field at the top and set checkin-needed if you don't have access. jwatt may be on the case for this bug even without that though.
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
Not on the case just yet, so if anyone gets to it first, feel free. Else maybe I'll get it pushed tomorrow if we have a better network connection at the conference and I can watch the tree.
(Reporter)

Comment 13

8 years ago
I'll land this along with another patch later today.
(Reporter)

Comment 14

8 years ago
Landed: http://hg.mozilla.org/mozilla-central/rev/ba366a4ba8fa
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Thanks Daniel. And thanks for the patch Felipe.
You need to log in before you can comment on or make changes to this bug.