Closed
Bug 995730
Opened 10 years ago
Closed 10 years ago
Convert xpcom/ to Gecko style
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: poiru, Assigned: poiru)
References
Details
Attachments
(20 files, 5 obsolete files)
503.34 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
874.96 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
472.98 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
281.09 KB,
patch
|
froydnj
:
review+
mccr8
:
review+
|
Details | Diff | Splinter Review |
95.78 KB,
patch
|
froydnj
:
review+
mccr8
:
review+
|
Details | Diff | Splinter Review |
166.00 KB,
patch
|
froydnj
:
review+
mccr8
:
feedback+
|
Details | Diff | Splinter Review |
453.38 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
302.14 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
297.13 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
29.94 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
272.55 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
165.96 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
53.33 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
63.76 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
142.10 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
265.99 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
262.17 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
199.23 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
275.88 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
170.38 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
Assignee | ||
Comment 1•10 years ago
|
||
I plan to first create large, whitespace-only patches to fix indentation in each subdirectory under xpcom/. Then, I'll create smaller patches for a few files at a time to address other parts of Gecko style adherence. Does this sound OK? This first patch fixes indentation for /xpcom/base/ (`diff -w` is empty with the exception of changed/added modelines).
Attachment #8405846 -
Flags: review?(benjamin)
Assignee | ||
Comment 2•10 years ago
|
||
This also removes trailing whitespace in reindented files.
Attachment #8405846 -
Attachment is obsolete: true
Attachment #8405846 -
Flags: review?(benjamin)
Attachment #8405880 -
Flags: review?(benjamin)
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8405881 -
Flags: review?(benjamin)
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Benjamin Smedberg [:bsmedberg] from comment #4) > Are you doing these by hand or with a tool? I'm using a combination of both. I use a tool to change indentation to 2 spaces and then fix up e.g. vertical parameter alignment by hand.
Flags: needinfo?(birunthan)
Assignee | ||
Comment 6•10 years ago
|
||
The previous iteration of this patch used a tool to some extent, but also involved a lot of manual work. This new patch was generated by running astyle, reviewing each hunk, and performing additional fine-tuning by hand. The following astyle options were used: --suffix=none --indent-col1-comments --indent=spaces=2 --indent-switches --max-instatement-indent=120 --keep-one-line-blocks --keep-one-line-statements If you want, I can attach a smaller diff consisting solely of the tweaks I made on top of astyle.
Attachment #8405880 -
Attachment is obsolete: true
Attachment #8405880 -
Flags: review?(benjamin)
Attachment #8407101 -
Flags: review?(benjamin)
Comment 7•10 years ago
|
||
Comment on attachment 8405881 [details] [diff] [review] Change xpcom/io/ to use 2 space indentation Bumping these to Nathan because I'm overbooked.
Attachment #8405881 -
Flags: review?(benjamin) → review?(nfroyd)
Updated•10 years ago
|
Attachment #8407101 -
Flags: review?(benjamin) → review?(nfroyd)
Assignee | ||
Comment 8•10 years ago
|
||
Comment on attachment 8405881 [details] [diff] [review] Change xpcom/io/ to use 2 space indentation Obsoleting this for now. If and when the xpcom/base/ patch lands, I'll redo this patch using the method described in comment 6.
Attachment #8405881 -
Attachment is obsolete: true
Attachment #8405881 -
Flags: review?(nfroyd)
Comment 9•10 years ago
|
||
Comment on attachment 8407101 [details] [diff] [review] Change xpcom/base/ to use 2 space indentation Review of attachment 8407101 [details] [diff] [review]: ----------------------------------------------------------------- Even fixing tabs! Nice work.
Attachment #8407101 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8416050 -
Flags: review?(nfroyd)
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8416051 -
Flags: review?(nfroyd)
Comment 12•10 years ago
|
||
Comment on attachment 8416051 [details] [diff] [review] Change xpcom/string/ to use 2 space indentation Review of attachment 8416051 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/string/public/nsCharTraits.h @@ +66,4 @@ > // 0xD7C0 == 0xD800 - 0x0080, > // ((c - 0x10000) >> 10) + 0xD800 can be simplified to > #define H_SURROGATE(c) char16_t(char16_t(uint32_t(c) >> 10) + \ > + char16_t(0xD7C0)) Nit: this seems like weird indentation; I think you want to shift this one space left.
Attachment #8416051 -
Flags: review?(nfroyd) → review+
Comment 13•10 years ago
|
||
Comment on attachment 8416050 [details] [diff] [review] Change xpcom/io/ to use 2 space indentation Review of attachment 8416050 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/io/nsLocalFileUnix.h @@ +29,5 @@ > * we need these for statfs() > */ > #ifdef HAVE_SYS_STATVFS_H > + #if defined(__osf__) && defined(__DECCXX) > + extern "C" int statvfs(const char *, struct statvfs *); I don't know if we really have a style guide for nested indentation like this. (Arguably, one could say that this falls under the "indentation per logic level" in the current style guide.) I guess fixing this is OK. @@ +36,4 @@ > #endif > > #ifdef HAVE_SYS_STATFS_H > + #include <sys/statfs.h> Note that we didn't indent the MOZ_WIDGET_COCOA bits 10 lines up from here. ;) ::: xpcom/io/nsMultiplexInputStream.h @@ +15,5 @@ > #include "nsIMultiplexInputStream.h" > > #define NS_MULTIPLEXINPUTSTREAM_CONTRACTID "@mozilla.org/io/multiplex-input-stream;1" > +#define NS_MULTIPLEXINPUTSTREAM_CID \ > +{ /* 565e3a2c-1dd2-11b2-8da1-b4cef17e568d */ \ I think technically the body of this macro should be indented two spaces.
Attachment #8416050 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 14•10 years ago
|
||
Comment 12 and comment 13 addressed and pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/4e27f466ebbb https://hg.mozilla.org/integration/mozilla-inbound/rev/7db230827e21 https://hg.mozilla.org/integration/mozilla-inbound/rev/81587bb1f916
Assignee | ||
Updated•10 years ago
|
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/4e27f466ebbb https://hg.mozilla.org/mozilla-central/rev/7db230827e21 https://hg.mozilla.org/mozilla-central/rev/81587bb1f916
Comment 16•10 years ago
|
||
poiru++
Comment 17•10 years ago
|
||
Fixup for breakage caused by a bad merge last night: https://hg.mozilla.org/integration/b2g-inbound/rev/9040b7692048
Assignee | ||
Comment 18•10 years ago
|
||
Attachment #8418882 -
Flags: review?(nfroyd)
Assignee | ||
Comment 19•10 years ago
|
||
Attachment #8418883 -
Flags: review?(nfroyd)
Comment 20•10 years ago
|
||
Comment on attachment 8418882 [details] [diff] [review] Fix style violations in xpcom/base/ Review of attachment 8418882 [details] [diff] [review]: ----------------------------------------------------------------- I reviewed CycleCollectedJSRuntime.{h,cpp} and nsCycleCollector.{h,cpp}, so Nathan doesn't need to review those unless he really wants to. Thanks for fixing this. I've been picking at the style for a while in the cycle collector, but there is still a ways to go as you can see. ::: xpcom/base/CycleCollectedJSRuntime.cpp @@ +303,5 @@ > return; > } > > if (AddToCCKind(js::GCThingTraceKind(aThing)) && > + xpc_IsGrayGCThing(aThing)) { I think the style is for multiline if conditions to have the brace on a new line (the way it was), but maybe that's Spidermonkey style that has specific exceptions like that... ::: xpcom/base/nsCycleCollector.cpp @@ +495,5 @@ > (mCurrent++)->ptrInfo = aEdge; > } > private: > // mBlockEnd points to space for null sentinel > + PtrInfoOrBlock* mCurrent, *mBlockEnd; You might as well just split this into two declarations. @@ +726,3 @@ > // mNext is the next value we want to return, unless mNext == mBlockEnd > // NB: mLast is a reference to allow enumerating while building! > + PtrInfo* mNext, *mBlockEnd, *& mLast; Please also just split this into three declarations.
Attachment #8418882 -
Flags: review+
Comment 21•10 years ago
|
||
Comment on attachment 8418883 [details] [diff] [review] Brace one-line control statements in xpcom/base/ Review of attachment 8418883 [details] [diff] [review]: ----------------------------------------------------------------- Thanks again for these changes! I reviewed the fixes in nsCycleCollector.cpp.
Attachment #8418883 -
Flags: review+
Comment 22•10 years ago
|
||
Comment on attachment 8418882 [details] [diff] [review] Fix style violations in xpcom/base/ Review of attachment 8418882 [details] [diff] [review]: ----------------------------------------------------------------- I defer to Andrew's review on the cycle collector parts. Apologies for all the nitpicky little things, but if I can't be nitpicky on a style patch, then when can I be nitpicky? ;) The biggest thing I noticed was splitting out the return type on function definitions onto its own line; I noted all the places I could see in the diff below. I don't doubt there are others. You're free to fix them if you like. ::: xpcom/base/AvailableMemoryTracker.cpp @@ +83,5 @@ > #define LOG4(m1, m2, m3, m4) > > #endif > > +void safe_write(const char* a) Split this, please. @@ +163,5 @@ > /** > * Fire a memory pressure event if it's been long enough since the last one we > * fired. > */ > bool MaybeScheduleMemoryPressureEvent() Split this, please. @@ +454,5 @@ > * free dirty pages held by jemalloc. > */ > NS_IMETHODIMP > +nsMemoryPressureWatcher::Observe(nsISupports* subject, const char* topic, > + const char16_t* data) Bonus points for fixing up arguments to have an 'a' prefix. Feel free to file a followup bug or followup patch. @@ +527,5 @@ > if (!PR_GetEnv("MOZ_PGO_INSTRUMENTED")) { > sKernel32Intercept.Init("Kernel32.dll"); > sKernel32Intercept.AddHook("VirtualAlloc", > reinterpret_cast<intptr_t>(VirtualAllocHook), > + (void**)&sVirtualAllocOrig); I think the best style would be to use reinterpret_cast here, rather than C-style casts. @@ +532,3 @@ > sKernel32Intercept.AddHook("MapViewOfFile", > reinterpret_cast<intptr_t>(MapViewOfFileHook), > + (void**)&sMapViewOfFileOrig); Likewise. ::: xpcom/base/ClearOnShutdown.h @@ +96,1 @@ > while ((observer = sShutdownObservers->popFirst())) { You could probably get away with, and make things clearer, by rewriting to say: while (ShutdownObserver* = ...) { } We don't use the assignment-in-conditions style often enough. ::: xpcom/base/nsCycleCollector.cpp @@ +495,5 @@ > (mCurrent++)->ptrInfo = aEdge; > } > private: > // mBlockEnd points to space for null sentinel > + PtrInfoOrBlock* mCurrent, *mBlockEnd; ++ on what Andrew said; pointer declarations should never be doubled up like this. ::: xpcom/base/nsDumpUtils.cpp @@ +128,5 @@ > PipeCallback aCallback) > { > MutexAutoLock lock(mSignalInfoLock); > > + for (SignalInfoArray::index_type i = 0; i < mSignalInfo.Length(); i++) { Might as well make it ++i while you're here. @@ +277,5 @@ > FifoWatcher::RegisterCallback(const nsCString& aCommand, FifoCallback aCallback) > { > MutexAutoLock lock(mFifoInfoLock); > > + for (FifoInfoArray::index_type i = 0; i < mFifoInfo.Length(); i++) { ++i here too. ::: xpcom/base/nsError.h @@ +177,5 @@ > * @return 0 or 1 (false/true with bool type for C++) > */ > > #ifdef __cplusplus > +inline uint32_t NS_FAILED_impl(nsresult _nsresult) Split this, please. @@ +237,3 @@ > return ((uint32_t(err) >> 16) - NS_ERROR_MODULE_BASE_OFFSET) & 0x1fff; > } > +inline bool NS_ERROR_GET_SEVERITY(nsresult err) And all of these little functions too, please. ::: xpcom/base/nsMacUtilsImpl.cpp @@ +87,5 @@ > > return (archString.IsEmpty() ? NS_ERROR_FAILURE : NS_OK); > } > > +NS_IMETHODIMP nsMacUtilsImpl::GetIsUniversalBinary(bool* aIsUniversalBinary) This declaration should be split: NS_IMETHODIMP nsMacUtilsImpl::... @@ +108,2 @@ > > NS_IMETHODIMP nsMacUtilsImpl::GetArchitecturesInBinary(nsAString& archString) So should this one. @@ +112,5 @@ > } > > /* readonly attribute boolean isTranslated; */ > // True when running under binary translation (Rosetta). > +NS_IMETHODIMP nsMacUtilsImpl::GetIsTranslated(bool* aIsTranslated) So should this one. ::: xpcom/base/nsMemoryImpl.cpp @@ +171,5 @@ > return NS_OK; > } > > // XXX need NS_IMPL_STATIC_ADDREF/RELEASE > +NS_IMETHODIMP_(MozExternalRefCountType) nsMemoryImpl::FlushEvent::AddRef() This declaration should be split: NS_IMETHODIMP(...) nsMemoryImpl::Flush... @@ +175,5 @@ > +NS_IMETHODIMP_(MozExternalRefCountType) nsMemoryImpl::FlushEvent::AddRef() > +{ > + return 2; > +} > +NS_IMETHODIMP_(MozExternalRefCountType) nsMemoryImpl::FlushEvent::Release() So should this one. ::: xpcom/base/nsMemoryReporterManager.cpp @@ +214,5 @@ > static nsresult > GetKinfoVmentrySelf(int64_t* aPrss, uint64_t* aMaxreg) > { > int cnt; > + struct kinfo_vmentry* vmmap, *kve; Please split this into two declarations. ::: xpcom/base/nsStackWalk.cpp @@ +61,2 @@ > { > + const char* name = reinterpret_cast<char*>(closure); While you're here, can you make this static_cast? @@ +138,4 @@ > MOZ_ASSERT(r == 0); > r = pthread_mutex_lock(&mutex); > MOZ_ASSERT(r == 0); > struct timespec abstime = {0, 1}; Might want to put spaces inside the braces here. @@ +236,5 @@ > > } > > // Routine to print an error message to standard error. > +void PrintError(const char* prefix) Split this, please. @@ +556,5 @@ > // use a separate walker thread. > WalkStackMain64(&data); > > if (data.pc_count > data.pc_size) { > + data.pcs = (void**)_alloca(data.pc_count * sizeof(void*)); This is amusing code...our always-allocated stack buffer is too small, so we're going to put an *even larger* buffer on the stack instead. @@ +561,3 @@ > data.pc_size = data.pc_count; > data.pc_count = 0; > + data.sps = (void**)_alloca(data.sp_count * sizeof(void*)); ...and we're going to do it *twice*. @@ +577,5 @@ > data.eventEnd, INFINITE, FALSE); > if (walkerReturn != WAIT_OBJECT_0 && !shouldBeThreadSafe) > PrintError("SignalObjectAndWait (1)"); > if (data.pc_count > data.pc_size) { > + data.pcs = (void**)_alloca(data.pc_count * sizeof(void*)); Not done yet! @@ +864,1 @@ > int aBufLen) Split this: void DemangleSymbol(...) and see if the arguments fit on one line. ::: xpcom/base/nsStackWalk.h @@ +21,5 @@ > // aSP will be the best approximation possible of what the stack pointer will be > // pointing to when the execution returns to executing that at that PC. > // If no approximation can be made it will be nullptr. > typedef void > +(* NS_WalkStackCallback)(void* aPC, void* aSP, void* aClosure); I don't know if we really have a style on function pointer types. I would tend to snuggle the * with the typedef name in cases like this. ::: xpcom/base/nsStatusReporterManager.cpp @@ +58,5 @@ > > static bool gStatusReportProgress = 0; > static int gNumReporters = 0; > > nsresult getStatus(nsACString& desc) Split this into two lines, please. ::: xpcom/base/nsTraceRefcnt.cpp @@ +645,5 @@ > #else > #define FOPEN_NO_INHERIT > #endif > > +static bool InitLog(const char* envVar, const char* msg, FILE** result) Split this, please. @@ +861,5 @@ > > extern "C" { > > #ifdef STACKWALKING_AVAILABLE > +static void PrintStackFrame(void* aPC, void* aSP, void* aClosure) This one too. @@ +1267,5 @@ > nsTraceRefcnt::Startup() > { > } > > +static void maybeUnregisterAndCloseFile(FILE*& f) This one too. ::: xpcom/base/nsTraceRefcnt.h @@ +30,1 @@ > int aBufLen); Does this not all fit on one line? ::: xpcom/base/nsUUIDGenerator.cpp @@ +66,2 @@ > mRBytes = 1; > + if ((unsigned long)RAND_MAX < (unsigned long)0x000000ff) Since you're touching these lines anyway, can you just make the constants suffixed with a UL rather than using casts? ::: xpcom/base/nsWindowsHelpers.h @@ +125,5 @@ > typedef nsAutoRef<HANDLE> nsAutoHandle; > typedef nsAutoRef<HMODULE> nsModuleHandle; > > +namespace { > + I find it ironic that a windows-specific header violates the bit of the style guide that says: "We prefer using 'static' instead of anonymous C++ namespaces. This may change once there is better debugger support (on Windows especially) for placing breakpoints, etc on code in anonymous namespaces." ::: xpcom/base/nscore.h @@ +307,5 @@ > */ > > +#define NS_PTR_TO_INT32(x) ((int32_t)(intptr_t)(x)) > +#define NS_PTR_TO_UINT32(x) ((uint32_t)(intptr_t)(x)) > +#define NS_INT32_TO_PTR(x) ((void *)(intptr_t)(x)) Nit: might as well eliminate the double space between macro and definition while you're at it.
Attachment #8418882 -
Flags: review?(nfroyd) → review+
Comment 23•10 years ago
|
||
Comment on attachment 8418883 [details] [diff] [review] Brace one-line control statements in xpcom/base/ Review of attachment 8418883 [details] [diff] [review]: ----------------------------------------------------------------- I was about to start commenting on fixing these in the style violations patch, but then I noticed/remembered you broke these out into a separate patch. :)
Attachment #8418883 -
Flags: review?(nfroyd) → review+
Comment 24•10 years ago
|
||
I'd personally be inclined to fold these patches before landing, so that |hg blame| gets polluted as little as possible.
Assignee | ||
Comment 25•10 years ago
|
||
(In reply to Nathan Froyd (:froydnj) from comment #22) > The biggest thing I noticed was splitting out the return type on function > definitions onto its own line; I noted all the places I could see in the > diff below. I don't doubt there are others. You're free to fix them if > you like. > Bonus points for fixing up arguments to have an 'a' prefix. Feel free to > file a followup bug or followup patch. Done and done. Note that I hacked together a tool based on Clang's libTooling for the changes in this patch. Because the tool actually parses the source code, it e.g. skips code that is ifdef-ed out. I used a combination of -D flags to cover as much as possible, but it might have missed some parts. (I also manually renamed a few arguments to be more descriptive.) > Apologies for all the nitpicky little things, but if I can't be nitpicky on > a style patch, then when can I be nitpicky? ;) No need to apologize. Thanks for the thorough reviews! (In reply to Nicholas Nethercote [:njn] from comment #24) > I'd personally be inclined to fold these patches before landing, so that |hg > blame| gets polluted as little as possible. Yep, will do. (In reply to Andrew McCreight [:mccr8] from comment #20) > ::: xpcom/base/CycleCollectedJSRuntime.cpp > @@ +303,5 @@ > > return; > > } > > > > if (AddToCCKind(js::GCThingTraceKind(aThing)) && > > + xpc_IsGrayGCThing(aThing)) { > > I think the style is for multiline if conditions to have the brace on a new > line (the way it was), but maybe that's Spidermonkey style that has specific > exceptions like that... AFAIK, that is specific to SpiderMonkey style due to 4-space indentation. In any case, it would be good to make this explicit in the Gecko style guide.
Attachment #8420507 -
Flags: review?(nfroyd)
Assignee | ||
Comment 26•10 years ago
|
||
Oops, uploaded wrong patch in comment 25.
Attachment #8420507 -
Attachment is obsolete: true
Attachment #8420507 -
Flags: review?(nfroyd)
Attachment #8420508 -
Flags: review?(nfroyd)
Comment 27•10 years ago
|
||
(In reply to Birunthan Mohanathas [:poiru] from comment #25) > Created attachment 8420507 [details] [diff] [review] > Fix function argument and return type style violations in xpcom/base/ > > (In reply to Nathan Froyd (:froydnj) from comment #22) > > The biggest thing I noticed was splitting out the return type on function > > definitions onto its own line; I noted all the places I could see in the > > diff below. I don't doubt there are others. You're free to fix them if > > you like. > > > Bonus points for fixing up arguments to have an 'a' prefix. Feel free to > > file a followup bug or followup patch. > > Done and done. > > Note that I hacked together a tool based on Clang's libTooling for the > changes in this patch. Because the tool actually parses the source code, it > e.g. skips code that is ifdef-ed out. I used a combination of -D flags to > cover as much as possible, but it might have missed some parts. The return type changes or the argument renaming changes, or something else, or all of the above? Can you share that code? I've been wondering if we're attempting to do too much with style rewrites (indentation, rewrapping, etc. etc.), and we should start by writing smaller lint-type tools that do One Thing Well: - argument naming checker; - kungFuDeathGrip checker; - return type formatter; - etc. Then people can start combining those as need be.
Flags: needinfo?(birunthan)
Assignee | ||
Comment 28•10 years ago
|
||
(In reply to Nathan Froyd (:froydnj) from comment #27) > The return type changes or the argument renaming changes, or something else, > or all of the above? Can you share that code? The tool renames arguments and formats the return type. At the monent, it is quite hacky and unfit for general use. I can share the code, but I'll have to do some cleaning first. In any case, because the tool is based on Clang's LibTooling, an invocation is going to have to look something like: ./reformat-func-decl $file -- \ -w -std=c++11 \ -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL \ -I/.../mozilla-central/obj-d/dist/include \ -I/.../mozilla-central/obj-d/dist/include/nspr \ -I/.../mozilla-central/obj-d/dist/include/js \ -I/.../mingw-w64-v3.1.0/mingw-w64-headers/include However, with proper mach support (e.g. by generating a Clang compilation database[0] during build), I think it would be feasible to create easy-to-use one-off tools like those you listed. Another approach would be to retrofit Clang's LibFormat (which, unlike LibTooling, does not parse headers, macros, etc.) for our needs. [0]: http://clang.llvm.org/docs/JSONCompilationDatabase.html
Flags: needinfo?(birunthan)
Comment 29•10 years ago
|
||
Comment on attachment 8420508 [details] [diff] [review] Fix function argument and return type style violations in xpcom/base/ Review of attachment 8420508 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for doing this. I'll ask Andrew to go over the cycle collector changes, to see if he has any complaints. I think it might have been worth it to split these up. The function argument positioning patches are uncontroversial. But the argument renaming ones illustrate one of the hazards of automated replacements: single-letter variable names come out poorly, and you also have instances like the TraceWeakMapping example below where the JSGCTraceKind argument names are logically connected to other arguments, but don't really appear so. What do you think about making a pass through to fix up the single-letter variable names a bit? I'm not entirely sure it'd be worth it, so I'm +0 on it. ::: xpcom/base/CycleCollectedJSRuntime.cpp @@ +171,5 @@ > > static void > +TraceWeakMapping(js::WeakMapTracer* aTrc, JSObject* aM, > + void* aK, JSGCTraceKind aKkind, > + void* aV, JSGCTraceKind aVkind) Perhaps they should be aMap, aKey and aValue (and then we would have aKeyKind and aValueKind). ::: xpcom/base/SystemMemoryReporter.cpp @@ +83,5 @@ > aOut.Assign(out); > } > > static bool > +IsNumeric(const char* aStr) Ah, so we can replace single-letter variable names with something more descriptive... ::: xpcom/base/nsMacUtilsImpl.h @@ -5,5 @@ > > #ifndef nsMacUtilsImpl_h___ > #define nsMacUtilsImpl_h___ > > -#include "nsIMacUtils.h" What's this getting removed for? Looks very much like you need it.
Attachment #8420508 -
Flags: review?(nfroyd)
Attachment #8420508 -
Flags: review+
Attachment #8420508 -
Flags: feedback?(continuation)
Comment 30•10 years ago
|
||
Comment on attachment 8420508 [details] [diff] [review] Fix function argument and return type style violations in xpcom/base/ Review of attachment 8420508 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for fixing this! As Nathan said, the single letter argument names look pretty bad in the aX form, so I've suggested some fixes for these. ::: xpcom/base/CycleCollectedJSRuntime.cpp @@ +99,5 @@ > void* aClosure); > > public: > IncrementalFinalizeRunnable(CycleCollectedJSRuntime* aRt, > + nsTArray<nsISupports*>& aMSupports, It looks like this was a typo in the original declaration. This should be aSupports, which matches the definition. @@ +171,5 @@ > > static void > +TraceWeakMapping(js::WeakMapTracer* aTrc, JSObject* aM, > + void* aK, JSGCTraceKind aKkind, > + void* aV, JSGCTraceKind aVkind) Yeah, like Nathan suggested, these should be aMap, aKey, aKeyKind, aValue, aValueKind. @@ +245,5 @@ > > static void > + FixWeakMappingGrayBits(js::WeakMapTracer* aTrc, JSObject* aM, > + void* aK, JSGCTraceKind aKkind, > + void* aV, JSGCTraceKind aVkind) Likewise here. @@ +337,5 @@ > return PL_DHASH_NEXT; > } > > NS_IMETHODIMP > +JSGCThingParticipant::Traverse(void* aP, Please change this to aPtr. @@ +354,5 @@ > // CycleCollectedJSRuntime. It should never be used directly. > static JSGCThingParticipant sGCThingCycleCollectorGlobal; > > NS_IMETHODIMP > +JSZoneParticipant::Traverse(void* aP, nsCycleCollectionTraversalCallback& aCb) Please change this to aPtr. @@ +759,5 @@ > } > > struct JsGcTracer : public TraceCallbacks > { > + virtual void Trace(JS::Heap<JS::Value>* aP, const char* aName, Please change the various aP things in this class to aPtr. ::: xpcom/base/CycleCollectedJSRuntime.h @@ +27,5 @@ > > class JSGCThingParticipant: public nsCycleCollectionParticipant > { > public: > + NS_IMETHOD_(void) Root(void* aN) Please change the aN arguments in this class to aPtr. @@ +51,5 @@ > { > public: > MOZ_CONSTEXPR JSZoneParticipant(): nsCycleCollectionParticipant() {} > > + NS_IMETHOD_(void) Root(void* aP) Please change the aP arguments in this class to aPtr. ::: xpcom/base/nsCycleCollector.cpp @@ +902,5 @@ > return out; > } > > static inline void > +ToParticipant(nsISupports* aS, nsXPCOMCycleCollectionParticipant** aCp); aS to aPtr @@ +1049,5 @@ > --aBuffer.mCount; > } > }; > > + void UnmarkRemainingPurple(Block* aB) aBlock please. @@ +1086,5 @@ > (uintptr_t(mFreeList->mNextInFreeList) & ~uintptr_t(1)); > return e; > } > > + MOZ_ALWAYS_INLINE void Put(void* aP, nsCycleCollectionParticipant* aCp, aP to aObject please. @@ +1100,3 @@ > } > > + void Remove(nsPurpleBufferEntry* aE) aEntry please. @@ +1258,5 @@ > CheckThreadSafety(); > mForgetSkippableCB = aForgetSkippableCB; > } > > + void Suspect(void* aN, nsCycleCollectionParticipant* aCp, aN -> aPtr please. @@ +1332,5 @@ > } > } > > public: > + void Walk(PtrInfo* aS0); aPi @@ +1376,4 @@ > } > > static inline void > +ToParticipant(nsISupports* aS, nsXPCOMCycleCollectionParticipant** aCp) aS to aPtr @@ +1386,5 @@ > } > > template <class Visitor> > MOZ_NEVER_INLINE void > +GraphWalker<Visitor>::Walk(PtrInfo* aS0) aS0 to aPi @@ +2281,5 @@ > + NS_IMETHOD_(void) NoteJSChild(void* aChild); > + > + NS_IMETHOD_(void) DescribeRefCountedNode(nsrefcnt aRefcount, > + const char* aObjname) {} > + NS_IMETHOD_(void) DescribeGCedNode(bool aIsmarked, aIsMarked please. @@ +2324,5 @@ > } > } > > static bool > +MayHaveChild(void* aO, nsCycleCollectionParticipant* aCp) aO to aObj please. @@ +3830,5 @@ > } // namespace mozilla > > > MOZ_NEVER_INLINE static void > +SuspectAfterShutdown(void* aN, nsCycleCollectionParticipant* aCp, aN to aPtr @@ +3850,5 @@ > } > } > > void > +NS_CycleCollectorSuspect3(void* aN, nsCycleCollectionParticipant* aCp, aN to aPtr
Attachment #8420508 -
Flags: feedback?(continuation) → feedback+
Assignee | ||
Comment 31•10 years ago
|
||
(In reply to Nathan Froyd (:froydnj) from comment #29) > What do you think about making a pass through to fix up the single-letter > variable names a bit? I'm not entirely sure it'd be worth it, so I'm +0 on > it. Andrew addressed most in comment 30, but I went ahead and fixed the names in other files as well. Pushed folded patch to try: https://tbpl.mozilla.org/?tree=Try&rev=cbe388d83a55 > ::: xpcom/base/nsMacUtilsImpl.h > @@ -5,5 @@ > > > > #ifndef nsMacUtilsImpl_h___ > > #define nsMacUtilsImpl_h___ > > > > -#include "nsIMacUtils.h" > > What's this getting removed for? Looks very much like you need it. It was a mistake.
Assignee | ||
Comment 32•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/74e5dc1deb8e (Try push: https://tbpl.mozilla.org/?tree=Try&rev=cbe388d83a55)
Assignee | ||
Comment 33•10 years ago
|
||
Attachment #8421931 -
Flags: review?(nfroyd)
Assignee | ||
Comment 34•10 years ago
|
||
Attachment #8421932 -
Flags: review?(nfroyd)
Assignee | ||
Comment 35•10 years ago
|
||
Attachment #8421933 -
Flags: review?(nfroyd)
Assignee | ||
Comment 37•10 years ago
|
||
Comment on attachment 8421931 [details] [diff] [review] Fix style violations in xpcom/io/ Review of attachment 8421931 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/io/nsEscape.cpp @@ +310,5 @@ > + 1008, 1023, 1023, 1023, 1023, 1023, 1023, 1023, 1023, 1023, 1023, 1023, 1023, 1023, 1023, 1023, /* 4x @ABCDEFGHIJKLMNO */ > + 1023, 1023, 1023, 1023, 1023, 1023, 1023, 1023, 1023, 1023, 1023, 896, 896, 896, 896, 1023, /* 5x PQRSTUVWXYZ[\]^_ */ > + 0, 1023, 1023, 1023, 1023, 1023, 1023, 1023, 1023, 1023, 1023, 1023, 1023, 1023, 1023, 1023, /* 6x `abcdefghijklmno */ > + 1023, 1023, 1023, 1023, 1023, 1023, 1023, 1023, 1023, 1023, 1023, 896, 1012, 896, 1023, 0, /* 7x pqrstuvwxyz{|}~ */ > + 0 /* 8x DEL */ Reverted this bit locally, please ignore.
Updated•10 years ago
|
Attachment #8421932 -
Flags: review?(nfroyd) → review+
Comment 38•10 years ago
|
||
Comment on attachment 8421931 [details] [diff] [review] Fix style violations in xpcom/io/ Review of attachment 8421931 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/io/nsLinebreakConverter.cpp @@ +88,5 @@ > ioLen *includes* a terminating null, if any > ----------------------------------------------------------------------------*/ > template<class T> > +static T* > +ConvertBreaks(const T* inSrc, int32_t& ioLen, const char* srcBreak, Nit: trailing whitespace. ::: xpcom/io/nsLocalFileCommon.cpp @@ +199,5 @@ > nsresult rv; > _retval.Truncate(0); > > + nsAutoString thisPath; > + nsAutoString fromPath; I don't think there's any reason to split these... @@ +204,5 @@ > + char16_t* thisNodes[kMaxNodesInPath]; > + char16_t* fromNodes[kMaxNodesInPath]; > + int32_t thisNodeCnt; > + int32_t fromNodeCnt; > + int32_t nodeIndex; ...or these int32_t's. No pointers or references here, so no issues with multiple declarations. ::: xpcom/io/nsStreamUtils.cpp @@ +338,5 @@ > if (mCloseSink) { > // close sink > if (mAsyncSink) > + mAsyncSink->CloseWithStatus(canceled ? cancelStatus > + : sourceCondition); This is inconsistent with the CloseWithStatus call that you modified just 10 lines earlier. I think the earlier one is more consistent with our style of operators at the end, sadly. ::: xpcom/io/nsWildCard.cpp @@ +256,3 @@ > while (start <= end) > map[lower(start++)] = 1; > return map[lower(val)]; It's so much fun to read through these patches and see what grotty code we have in places.
Attachment #8421931 -
Flags: review?(nfroyd) → review+
Comment 39•10 years ago
|
||
Comment on attachment 8421933 [details] [diff] [review] Fix argument names in xpcom/io/ Review of attachment 8421933 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the changes below. ::: xpcom/io/nsMultiplexInputStream.cpp @@ +672,5 @@ > } > > nsresult > +nsMultiplexInputStreamConstructor(nsISupports* aOuter, > + REFNSIID aIid, This should probably be aIID instead (and similarly for any other aIid instances that I didn't mention). ::: xpcom/io/nsMultiplexInputStream.h @@ +23,5 @@ > {0x8d, 0xa1, 0xb4, 0xce, 0xf1, 0x7e, 0x56, 0x8d} \ > } > > +extern nsresult nsMultiplexInputStreamConstructor(nsISupports* aOuter, > + REFNSIID aIid, ...like this one. ::: xpcom/io/nsNativeCharsetUtils.cpp @@ +15,5 @@ > #include "nsReadableUtils.h" > #include "nsString.h" > > nsresult > +NS_CopyNativeToUnicode(const nsACString& aInput, nsAString& aOutput) Nit: nsAString& aOutput (less space).
Attachment #8421933 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 40•10 years ago
|
||
This contains a few additional changes to xpcom/base/ that were missed in the previous set of patches due to ifdefs and oversight. Also, thank you for the quick and meticulous reviews of these tiresome patches! (In reply to Nathan Froyd (:froydnj) from comment #38) > ::: xpcom/io/nsWildCard.cpp > @@ +256,3 @@ > > while (start <= end) > > map[lower(start++)] = 1; > > return map[lower(val)]; > > It's so much fun to read through these patches and see what grotty code we > have in places. Hah, yeah.
Attachment #8422664 -
Flags: review?(nfroyd)
Comment 41•10 years ago
|
||
Comment on attachment 8422664 [details] [diff] [review] Fix style violations in xpcom/base/ (part 2) Review of attachment 8422664 [details] [diff] [review]: ----------------------------------------------------------------- I *just* got done reviewing the last batch of these when this one came in. Have a quick review.
Attachment #8422664 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 42•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e836b7e11013 https://hg.mozilla.org/integration/mozilla-inbound/rev/f20eb9631869 (Try push: https://tbpl.mozilla.org/?tree=Try&rev=6d83c08b053c)
https://hg.mozilla.org/mozilla-central/rev/e836b7e11013 https://hg.mozilla.org/mozilla-central/rev/f20eb9631869
Assignee | ||
Comment 44•10 years ago
|
||
Attachment #8424552 -
Flags: review?(nfroyd)
Assignee | ||
Comment 45•10 years ago
|
||
Attachment #8424553 -
Flags: review?(nfroyd)
Assignee | ||
Comment 46•10 years ago
|
||
Attachment #8424554 -
Flags: review?(nfroyd)
Assignee | ||
Comment 47•10 years ago
|
||
Attachment #8424555 -
Flags: review?(nfroyd)
Assignee | ||
Comment 48•10 years ago
|
||
Attachment #8424556 -
Flags: review?(nfroyd)
Comment 49•10 years ago
|
||
Comment on attachment 8424552 [details] [diff] [review] Fix style violations in xpcom/string/ Review of attachment 8424552 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the changes below. ::: xpcom/string/public/nsReadableUtils.h @@ +414,3 @@ > int32_t > CompareUTF8toUTF16(const nsASingleFragmentCString& aUTF8String, > const nsASingleFragmentString& aUTF16String); This (and any others herein) should be fixed to be: int32_t CompareUTF8toUTF16(...); @@ +415,5 @@ > CompareUTF8toUTF16(const nsASingleFragmentCString& aUTF8String, > const nsASingleFragmentString& aUTF16String); > > void > AppendUCS4ToUTF16(const uint32_t aSource, nsAString& aDest); Likewise. ::: xpcom/string/public/nsString.h @@ +55,5 @@ > class NS_LossyConvertUTF16toASCII : public nsAutoCString > { > public: > explicit > + NS_LossyConvertUTF16toASCII(const char16_t* aString) I think the |explicit| should be on the same line here and throughout. ::: xpcom/string/public/nsStringIterator.h @@ +54,3 @@ > > pointer > start() const I guess all of the methods in this class should be reformatted, e.g. this one should be: pointer start() const ? @@ +89,2 @@ > self_type& > operator++() And e.g. this one should be: self_type& operator++() { ... } @@ +191,3 @@ > > pointer > start() const Likewise for this class. ::: xpcom/string/public/nsTSubstring.h @@ +357,5 @@ > */ > > + void NS_FASTCALL Assign(char_type c); > + bool NS_FASTCALL Assign(char_type c, > + const fallible_t&) NS_WARN_UNUSED_RESULT; I think placing the NS_WARN_UNUSED_RESULT at the beginning of the declaration, the same way we now do MOZ_WARN_UNUSED_RESULT, would be better. Likewise for the rest of this file and patch. @@ +415,5 @@ > // non-constant char array variable. Use AssignASCII for those. > // There are not fallible version of these methods because they only really > // apply to small allocations that we wouldn't want to check anyway. > template<int N> > + void AssignLiteral(const char_type(&str)[N]) I'm not totally convinced that shoving together |char_type(&str)| is appropriate here (and elsewhere in this patch where we use this trick). I think that either the previous: const char_type (&str)[N] is OK or the slightly unusual: const char_type(& str)[N] is better than the current patch. WDYT? @@ +1016,5 @@ > // > // NOTE: these flags are declared public _only_ for convenience inside > // the string implementation. > > + enum { Per dev-platform discussion, this brace should be on the next line as it currently is. @@ +1066,5 @@ > // mutually exclusive with F_SHARED, F_OWNED, and F_FIXED. > // > }; > > +int NS_FASTCALL Compare(const nsTSubstring_CharT::base_string_type& lhs, This should be: int NS_FASTCALL Compare(...) @@ +1073,5 @@ > > > inline > +bool > +operator!=(const nsTSubstring_CharT::base_string_type& lhs, This should be: inline bool operator!=(...) and so on for the rest of the overloaded operators in this file. ::: xpcom/string/public/nsXPCOMStrings.h @@ +142,5 @@ > * This function may allocate additional memory for aContainer. When > * aContainer is no longer needed, NS_StringContainerFinish should be called. > */ > XPCOM_API(nsresult) > +NS_StringContainerInit(nsStringContainer& aContainer); I guess technically this declaration and all following ones should be: XPCOM_API(nsresult) NS_StringContainerInit(...); ::: xpcom/string/src/nsTSubstring.cpp @@ +839,5 @@ > > return len; > } > > +void nsTSubstring_CharT::AppendPrintf(const char* format, ...) Break this after |void|. @@ +851,4 @@ > va_end(ap); > } > > +void nsTSubstring_CharT::AppendPrintf(const char* format, va_list ap) Here too.
Attachment #8424552 -
Flags: review?(nfroyd) → review+
Comment 50•10 years ago
|
||
Comment on attachment 8424553 [details] [diff] [review] Fix argument names in xpcom/string/ Review of attachment 8424553 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/string/src/nsStringComparator.cpp @@ +23,5 @@ > int > +nsCaseInsensitiveCStringComparator::operator()(const char_type* aLhs, > + const char_type* aRhs, > + uint32_t aLLength, > + uint32_t aRLength) const I think these should be |aLhsLength| and |aRhsLength|.
Attachment #8424553 -
Flags: review?(nfroyd) → review+
Comment 51•10 years ago
|
||
Comment on attachment 8424554 [details] [diff] [review] Use 2 space indentation in xpcom/threads/ Review of attachment 8424554 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/threads/HangMonitor.cpp @@ +331,2 @@ > if (activityType == kGeneralActivity) { > activityType = IsUIMessageWaiting() ? kActivityUIAVail : Nit: might as well remove this trailing whitespace too. ::: xpcom/threads/HangMonitor.h @@ +1,2 @@ > +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ > +/* vim: set ts=8 sts=2 et sw=2 tw=80: */ It is extremely weird that this whole file is showing up in the diff for this one-line change. ::: xpcom/threads/TimerThread.cpp @@ +227,5 @@ > if (!mTimers.IsEmpty()) { > timer = mTimers[0]; > > if (now >= timer->mTimeout || forceRunThisTimer) { > +next: I think this arguably belongs where it was before.
Attachment #8424554 -
Flags: review?(nfroyd) → review+
Comment 52•10 years ago
|
||
Comment on attachment 8424555 [details] [diff] [review] Fix style violations in xpcom/threads/ Review of attachment 8424555 [details] [diff] [review]: ----------------------------------------------------------------- The attachment says xpcom/threads/, but the patch says xpcom/string/.
Attachment #8424555 -
Flags: review?(nfroyd)
Updated•10 years ago
|
Attachment #8424556 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 53•10 years ago
|
||
(In reply to Nathan Froyd (:froydnj) from comment #52) > The attachment says xpcom/threads/, but the patch says xpcom/string/. Apologies, here is the correct patch. (Guess I should start using bzexport.) (In reply to Nathan Froyd (:froydnj) from comment #51) > ::: xpcom/threads/HangMonitor.h > @@ +1,2 @@ > > +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ > > +/* vim: set ts=8 sts=2 et sw=2 tw=80: */ > > It is extremely weird that this whole file is showing up in the diff for > this one-line change. That's due to line ending changes (CR+LF to LF). (In reply to Nathan Froyd (:froydnj) from comment #49) > I'm not totally convinced that shoving together |char_type(&str)| is > appropriate here (and elsewhere in this patch where we use this trick). I > think that either the previous: > > const char_type (&str)[N] > > is OK or the slightly unusual: > > const char_type(& str)[N] > > is better than the current patch. WDYT? I'll change it back to the previous one.
Attachment #8424555 -
Attachment is obsolete: true
Attachment #8425587 -
Flags: review?(nfroyd)
Comment 54•10 years ago
|
||
Comment on attachment 8425587 [details] [diff] [review] Fix style violations in xpcom/threads/ Review of attachment 8425587 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/threads/BackgroundHangMonitor.cpp @@ +119,5 @@ > { > /* We can tolerate init() failing. > The if block turns off warn_unused_result. */ > + if (!sTlsKey.init()) > + { This if block is braced improperly. But even better would be just: (void) sTlsKey.init(); I think that satisfies the compiler and its warnings. ::: xpcom/threads/HangMonitor.cpp @@ +393,5 @@ > } > } > > +} > +} // namespace mozilla::HangMonitor I think we comment each namespace separately, so: } // namespace HangMonitor } // namespace mozilla ::: xpcom/threads/HangMonitor.h @@ +51,5 @@ > */ > void Suspend(); > > +} > +} // namespace mozilla::HangMonitor Likewise with the namespaces. ::: xpcom/threads/ThreadStackHelper.cpp @@ +56,2 @@ > #endif > + mStackBuffer() This is an edge case; I think I'd keep the members the way they were before. ::: xpcom/threads/nsThread.cpp @@ +177,5 @@ > NS_INTERFACE_MAP_ENTRY(nsIEventTarget) > NS_INTERFACE_MAP_ENTRY(nsISupportsPriority) > NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIThread) > + if (aIID.Equals(NS_GET_IID(nsIClassInfo))) > + { This brace should not be moved. @@ +306,1 @@ > PRThreadPriority(ChaosMode::randomUint32LessThan(PR_PRIORITY_LAST + 1))); I think rather than this it'd be better to introduce a temporary for the ChaosMode random integer. This is another edge case, so I'm less sure of what should happen here. ::: xpcom/threads/nsTimerImpl.h @@ +114,2 @@ > > union CallbackUnion { Per dev-platform, please move this to the next line.
Attachment #8425587 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 55•10 years ago
|
||
(In reply to Nathan Froyd (:froydnj) from comment #49) > @@ +357,5 @@ > > */ > > > > + void NS_FASTCALL Assign(char_type c); > > + bool NS_FASTCALL Assign(char_type c, > > + const fallible_t&) NS_WARN_UNUSED_RESULT; > > I think placing the NS_WARN_UNUSED_RESULT at the beginning of the > declaration, the same way we now do MOZ_WARN_UNUSED_RESULT, would be better. > Likewise for the rest of this file and patch. Just to confirm, do you mean this? NS_WARN_UNUSED_RESULT bool NS_FASTCALL Assign(char_type c, const fallible_t&); Or perhaps this? NS_WARN_UNUSED_RESULT bool NS_FASTCALL Assign(char_type c, const fallible_t&);
Assignee | ||
Comment 56•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb3b772bac41 https://hg.mozilla.org/integration/mozilla-inbound/rev/747dc2140460
Comment 57•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/eb3b772bac41 https://hg.mozilla.org/mozilla-central/rev/747dc2140460
Assignee | ||
Comment 58•10 years ago
|
||
Attachment #8433333 -
Flags: review?(nfroyd)
Assignee | ||
Comment 59•10 years ago
|
||
Attachment #8433334 -
Flags: review?(nfroyd)
Updated•10 years ago
|
Attachment #8433333 -
Flags: review?(nfroyd) → review+
Comment 60•10 years ago
|
||
Comment on attachment 8433334 [details] [diff] [review] Change xpcom/glue/ to use 2 space indentation (part 2) Review of attachment 8433334 [details] [diff] [review]: ----------------------------------------------------------------- The nsCOMPtr.h changes alone are worth it.
Attachment #8433334 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 61•10 years ago
|
||
After the recent one-liner style discussions[0], do you think the following style is OK going forward? class Foo { Foo() {} Foo() : mBar(true) { Baz(); } bool GetBar() { return mBar; } }; [0]: https://groups.google.com/d/msg/mozilla.dev.platform/0ixYMnOKBog/PZIxruoAJPgJ
Flags: needinfo?(nfroyd)
Comment 62•10 years ago
|
||
(In reply to Birunthan Mohanathas [:poiru] from comment #61) > After the recent one-liner style discussions[0], do you think the following > style is OK going forward? > > class Foo > { > Foo() {} > Foo() : mBar(true) { Baz(); } > bool GetBar() { return mBar; } > }; Sure.
Flags: needinfo?(nfroyd)
Assignee | ||
Comment 63•10 years ago
|
||
Attachment #8436366 -
Flags: review?(nfroyd)
Assignee | ||
Comment 64•10 years ago
|
||
Attachment #8436367 -
Flags: review?(nfroyd)
Assignee | ||
Comment 65•10 years ago
|
||
Attachment #8436368 -
Flags: review?(nfroyd)
Comment 66•10 years ago
|
||
Birunthan, I suggest continuing this work in a new bug. It's fine to have multiple patches in one bug, but my rule of thumb is that if they don't land fairly close together, then a new bug is appropriate. Otherwise the "target milestone" field becomes useless, because multiple patches from one bug get landed in two or more Firefox versions. Also, when bugs get very long they become harder to read and follow. (For example, I converted part of MFBT's style in bug 1014377, and I'll do the remaining chunks in separate bugs.) Thanks!
Assignee | ||
Comment 67•10 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #66) > Birunthan, I suggest continuing this work in a new bug. It's fine to have > multiple patches in one bug, but my rule of thumb is that if they don't land > fairly close together, then a new bug is appropriate. Otherwise the "target > milestone" field becomes useless, because multiple patches from one bug get > landed in two or more Firefox versions. Also, when bugs get very long they > become harder to read and follow. Will do. I'll use bug 1022456 for subsequent patches.
Updated•10 years ago
|
Attachment #8436366 -
Flags: review?(nfroyd) → review+
Comment 68•10 years ago
|
||
Comment on attachment 8436367 [details] [diff] [review] Fix style violations in xpcom/glue/ (part 2) Review of attachment 8436367 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/glue/DeadlockDetector.h @@ +256,5 @@ > */ > void AddOrder(PLHashEntry* aLT, PLHashEntry* aGT) > { > static_cast<OrderingEntry*>(aLT->value)->mOrderedLT > + .InsertElementSorted(aGT); This change is incorrect. ::: xpcom/glue/nsBaseHashtable.h @@ +220,5 @@ > + typedef size_t (*SizeOfEntryExcludingThisFun)( > + KeyType aKey, > + const DataType& aData, > + mozilla::MallocSizeOf mallocSizeOf, > + void* userArg); I think the previous wrapping is better than this one. (This is one of the cases I would hate to teach an automatic formatter about...) ::: xpcom/glue/nsThreadUtils.h @@ +56,5 @@ > * Indicates that the given name is not unique. > */ > +extern NS_COM_GLUE NS_METHOD NS_NewThread( > + nsIThread** result, nsIRunnable* initialEvent = nullptr, > + uint32_t stackSize = nsIThreadManager::DEFAULT_STACK_SIZE); This was better the way it was before. @@ +116,5 @@ > * If event is null. > */ > +extern NS_COM_GLUE NS_METHOD NS_DispatchToMainThread( > + nsIRunnable* event, > + uint32_t dispatchFlags = NS_DISPATCH_NORMAL); Likewise. @@ +136,5 @@ > * Pass PR_INTERVAL_NO_TIMEOUT to specify no timeout. > */ > +extern NS_COM_GLUE NS_METHOD NS_ProcessPendingEvents( > + nsIThread* thread, > + PRIntervalTime timeout = PR_INTERVAL_NO_TIMEOUT); Likewise. ::: xpcom/glue/pldhash.h @@ +462,5 @@ > * the entry is marked so that PL_DHASH_ENTRY_IS_FREE(entry). This operation > * returns null unconditionally; you should ignore its return value. > */ > +NS_COM_GLUE PLDHashEntryHdr* PL_DHASH_FASTCALL PL_DHashTableOperate( > + PLDHashTable* table, const void* key, PLDHashOperator op); I think it is slightly preferable to keep PL_DHashTableOperate on its own line so that you don't have to wrap the argument list in odd ways. @@ +522,4 @@ > > +NS_COM_GLUE uint32_t PL_DHashTableEnumerate(PLDHashTable* table, > + PLDHashEnumerator etor, > + void* arg); Likewise for not wrapping here.
Attachment #8436367 -
Flags: review?(nfroyd) → review+
Comment 69•10 years ago
|
||
Comment on attachment 8436368 [details] [diff] [review] Fix style violations in xpcom/glue/ (part 3) Review of attachment 8436368 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/glue/nsCOMPtr.h @@ +428,5 @@ > + const nsGetServiceByContractID, const nsIID&); > + NS_COM_GLUE void NS_FASTCALL assign_from_gs_contractid_with_error( > + const nsGetServiceByContractIDWithError&, const nsIID&); > + NS_COM_GLUE void NS_FASTCALL assign_from_helper(const nsCOMPtr_helper&, > + const nsIID&); I think rather than wrapping all these differently, I'd stay within line-length limits by doing: NS_COM_GLUE void NS_FASTCALL assign_with_AddRef(...); NS_COM_GLUE void NS_FASTCALL assign_from_qi(...); and so forth. (I hope those all fit on one line!) ::: xpcom/glue/nsStringAPI.h @@ +177,3 @@ > #ifdef MOZ_USE_CHAR16_WRAPPER > + NS_HIDDEN_(void) Append(char16ptr_t data, > + size_type length = size_type(-1)) Please don't wrap this. @@ +1256,5 @@ > static_assert(char16_t(-1) > char16_t(0), "char16_t must be unsigned"); > > #define NS_MULTILINE_LITERAL_STRING(s) nsDependentString(reinterpret_cast<const nsAString::char_type*>(s), uint32_t((sizeof(s)/2)-1)) > #define NS_MULTILINE_LITERAL_STRING_INIT(n,s) n(reinterpret_cast<const nsAString::char_type*>(s), uint32_t((sizeof(s)/2)-1)) > #define NS_NAMED_MULTILINE_LITERAL_STRING(n,s) const nsDependentString n(reinterpret_cast<const nsAString::char_type*>(s), uint32_t((sizeof(s)/2)-1)) I would suggest that you wrap these, but I don't think there's any nice way to wrap them--at least not this last one. Maybe with a helper macro for the reinterpret_cast bit. @@ +1269,5 @@ > #define NS_NAMED_LITERAL_STRING(n,s) NS_NAMED_MULTILINE_LITERAL_STRING(n, MOZ_UTF16(s)) > > #define NS_LITERAL_CSTRING(s) static_cast<const nsDependentCString&>(nsDependentCString(s, uint32_t(sizeof(s)-1))) > #define NS_LITERAL_CSTRING_INIT(n,s) n(s, uint32_t(sizeof(s)-1)) > #define NS_NAMED_LITERAL_CSTRING(n,s) const nsDependentCString n(s, uint32_t(sizeof(s)-1)) Wrap these or delete whitespace, please. ::: xpcom/glue/nsTArray.h @@ +160,5 @@ > struct nsTArrayFallibleAllocator : nsTArrayFallibleAllocatorBase > { > + static void* Malloc(size_t size) { return moz_malloc(size); } > + static void* Realloc(void* ptr, size_t size) > + { return moz_realloc(ptr, size); } If it can't fit on one line, please use the multiple-line style. @@ +170,5 @@ > struct nsTArrayInfallibleAllocator : nsTArrayInfallibleAllocatorBase > { > + static void* Malloc(size_t size) { return moz_xmalloc(size); } > + static void* Realloc(void* ptr, size_t size) > + { return moz_xrealloc(ptr, size); } Likewise. @@ +1291,5 @@ > // Check that the previous assert didn't overflow > MOZ_ASSERT(start <= start + count, "Start index plus length overflows"); > DestructRange(start, count); > + this->ShiftData(start, count, 0, > + sizeof(elem_type), MOZ_ALIGNOF(elem_type)); No need to wrap this.
Attachment #8436368 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 70•10 years ago
|
||
Will continue work in bug 1022456.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•