Closed
Bug 1391803
Opened 7 years ago
Closed 7 years ago
Classes should use nsStringFwd.h rather than ad-hoc definitions
Categories
(Core :: XPCOM, enhancement, P3)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: erahm, Assigned: erahm)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
75.09 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
We should not be declaring forward declarations for nsString classes directly, instead we should use nsStringFwd.h. This will make changing the underlying types easier.
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8898986 -
Flags: review?(nfroyd)
Assignee | ||
Comment 2•7 years ago
|
||
Sorry for the churn, this includes changes to nsStringFwd.h as well.
Attachment #8898999 -
Flags: review?(nfroyd)
Assignee | ||
Updated•7 years ago
|
Attachment #8898986 -
Attachment is obsolete: true
Attachment #8898986 -
Flags: review?(nfroyd)
Assignee | ||
Comment 3•7 years ago
|
||
Comment on attachment 8898999 [details] [diff] [review] Use nsStringFwd.h for forward declaring string classes Review of attachment 8898999 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/security/test/gtest/TestCSPParser.cpp @@ -8,5 @@ > > #include <string.h> > #include <stdlib.h> > > -#ifndef MOZILLA_INTERNAL_API This is built as a xul gtest... ::: widget/tests/TestChromeMargin.cpp @@ +16,5 @@ > > #include "TestHarness.h" > > #ifndef MOZILLA_INTERNAL_API > +#error This test needs MOZILLA_INTERNAL_API (see bug 652123) Technically we don't build this test at all. ::: xpcom/build/nsGeckoProcessType.h @@ +1,2 @@ > +#ifndef xpcom_build_nsGeckoProcessType_h > +#define xpcom_build_nsGeckoProcessType_h I split this out so the sandbox and plugin process can include it directly without needing nsXULApi which in turn included nsStringFwd which used to assert MOZILLA_INTERNAL_API. Since I changed nsStringFwd I can probably revert it. ::: xpcom/string/nsStringFwd.h @@ -12,5 @@ > #include "nscore.h" > > -#ifndef MOZILLA_INTERNAL_API > -#error Internal string headers are not available from external-linkage code. > -#endif This was more hassle than it's worth. Given the concrete classes assert this I don't think it's a big deal if non-internal API stuff gets these forward declarations.
Comment 4•7 years ago
|
||
Comment on attachment 8898999 [details] [diff] [review] Use nsStringFwd.h for forward declaring string classes Review of attachment 8898999 [details] [diff] [review]: ----------------------------------------------------------------- I have questions, below; I'm also curious if we can just discard the nsGeckoProcessType.h (should probably be GeckoProcessType.h nowadays?) changes, as you already mentioned in your comment on that file. ::: dom/security/test/gtest/TestCSPParser.cpp @@ -13,5 @@ > -// some of the includes make use of internal string types > -#define nsAString_h___ > -#define nsString_h___ > -#define nsStringFwd_h___ > -#define nsReadableUtils_h___ Yuuuuck. ::: xpcom/build/nsGeckoProcessType.h @@ +17,5 @@ > + GeckoProcessType_End, > + GeckoProcessType_Invalid = GeckoProcessType_End > +}; > + > +static const char* const kGeckoProcessTypeString[] = { Reverting the file would be fine, as you mention above, but if we're going to keep it, let's move the definition of kGeckoProcessTypeString into a separate .cpp, so we don't wind up with a separate copy in every file that uses it? I guess we only use the array in a single file at the moment anyway and we currently define the array in a header, but defining static arrays in headers is bad form regardless. ::: xpcom/build/nsXPCOM.h @@ +156,5 @@ > */ > > #ifdef __cplusplus > > +XPCOM_API(nsresult) NS_NewLocalFile(const nsTSubstring<char16_t>& aPath, Can we not use nsStringFwd.h above if __cplusplus, and then use the normal nsA*String types here and below? I am confused.
Attachment #8898999 -
Flags: review?(nfroyd) → feedback+
Updated•7 years ago
|
Priority: -- → P3
Assignee | ||
Comment 5•7 years ago
|
||
This removes the XUL API changes in favor of just removing the internal API restriction from nsStringFwd.h
Attachment #8899974 -
Flags: review?(nfroyd)
Assignee | ||
Updated•7 years ago
|
Attachment #8898999 -
Attachment is obsolete: true
Assignee | ||
Comment 6•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=002b38cfb0a33c35c3197f110d4885bb5fdfc926
Assignee | ||
Comment 7•7 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #4) > Comment on attachment 8898999 [details] [diff] [review] > > ::: xpcom/build/nsGeckoProcessType.h > @@ +17,5 @@ > > + GeckoProcessType_End, > > + GeckoProcessType_Invalid = GeckoProcessType_End > > +}; > > + > > +static const char* const kGeckoProcessTypeString[] = { > > Reverting the file would be fine, as you mention above, but if we're going > to keep it, let's move the definition of kGeckoProcessTypeString into a > separate .cpp, so we don't wind up with a separate copy in every file that > uses it? > > I guess we only use the array in a single file at the moment anyway and we > currently define the array in a header, but defining static arrays in > headers is bad form regardless. I chose to just revert it for simplicity. > ::: xpcom/build/nsXPCOM.h > @@ +156,5 @@ > > */ > > > > #ifdef __cplusplus > > > > +XPCOM_API(nsresult) NS_NewLocalFile(const nsTSubstring<char16_t>& aPath, > > Can we not use nsStringFwd.h above if __cplusplus, and then use the normal > nsA*String types here and below? I am confused. Yeah we can use it now that the internal API restriction is removed, I'll update it.
Comment 8•7 years ago
|
||
Comment on attachment 8899974 [details] [diff] [review] Use nsStringFwd.h for forward declaring string classes Review of attachment 8899974 [details] [diff] [review]: ----------------------------------------------------------------- Great, thank you!
Attachment #8899974 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 9•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/397cfed5073f34740aed9e20460810316ee8ec25 Bug 1391803 - Use nsStringFwd.h for forward declaring string classes. r=froydnj
Assignee | ||
Updated•7 years ago
|
Blocks: StringCleaning
Comment 10•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/397cfed5073f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•3 years ago
|
Component: String → XPCOM
You need to log in
before you can comment on or make changes to this bug.
Description
•