1022456 - Convert more of xpcom/ to Gecko style

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[poiru]

See bug 995730.

Comment 1

[poiru]

Attachment: Fix style violations in xpcom/glue/

This is a folded patch of attachment 8433333 [details] [diff] [review], attachment 8433334 [details] [diff] [review], attachment 8436366 [details] [diff] [review], attachment 8436367 [details] [diff] [review], and attachment 8436368 [details] [diff] [review] from bug 995730 with review comments addressed.

Comment 2

[poiru]

Attachment: Brace one-line control statements in xpcom/glue/

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8443703 [details] [diff] [review]
Brace one-line control statements in xpcom/glue/

Review of attachment 8443703 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/glue/DeadlockDetector.h
@@ +492,5 @@
>     *
>     * @precondition |aStart != aTarget|
>     */
> +  ResourceAcquisitionArray* GetDeductionChain(const PLHashEntry* aStart,
> +                                              const PLHashEntry* aTarget)

Hunk in the wrong patch?

::: xpcom/glue/nsTextFormatter.cpp
@@ -900,5 @@
>       * the various sprintf() implementations are inconsistent
>       * on this feature.
>       */
>      while ((c == '-') || (c == '+') || (c == ' ') || (c == '0')) {
> -      if (c == '-') flags |= _LEFT;

Whoa, whoa, no separate patch for converting single-line statements to multi-line statements? ;)

::: xpcom/glue/nsVoidArray.cpp
@@ +645,5 @@
>  }
>  
>  bool
>  nsVoidArray::EnumerateForwards(nsVoidArrayEnumFuncConst aFunc,
> +                               void* aData) const

Hunk in the wrong patch?

Comment 4

[poiru]

Attachment: Fix argument names in xpcom/glue/

Comment 5

[poiru]

Attachment: Fix more style violations in xpcom/glue/

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8446095 [details] [diff] [review]
Fix more style violations in xpcom/glue/

Review of attachment 8446095 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the changes below.

::: xpcom/glue/BlockingResourceBase.cpp
@@ +96,5 @@
>  
>  void
>  BlockingResourceBase::CheckAcquire(const CallStack& aCallContext)
>  {
> +  if (mDDEntry->mType == eCondVar) {

FWIW, I don't know that we would consider these style violations.

::: xpcom/glue/EnumeratedArrayCycleCollection.h
@@ +27,1 @@
>            MOZ_TEMPLATE_ENUM_CLASS_ENUM_TYPE(IndexType) SizeAsEnumValue,

If you're going to fix the spacing for the opening template argument, you need to fix the indentation of the remaining template arguments.

::: xpcom/glue/nsCycleCollectionNoteChild.h
@@ +57,1 @@
>            bool IsXPCOM = mozilla::IsBaseOf<nsISupports, T>::value>

Likewise here on the indentation.

::: xpcom/glue/nsIInterfaceRequestorUtils.cpp
@@ +12,5 @@
>    nsresult status;
>  
>    if (mSource) {
> +    nsCOMPtr<nsIInterfaceRequestor> factoryPtr =
> +      do_QueryInterface(mSource, &status);

Seems like we could drop the &status parameter (since we'll assign it regardless below) and then maybe we wouldn't have to wrap this line?

::: xpcom/glue/nsStringAPI.h
@@ +1444,5 @@
>  
>  inline const nsDependentSubstring
>  Substring(const char16_t* aStart, const char16_t* aEnd)
>  {
> +  NS_ABORT_IF_FALSE(uint32_t(aEnd - aStart) == uintptr_t(aEnd - aStart), 

Nit: trailing whitespace.

::: xpcom/glue/nsThreadUtils.h
@@ +251,1 @@
>            typename ReturnType = void,

Indentation.

Comment 7

[poiru]

Attachment: Fix modelines in xpcom/{base,glue,io,string,threads}/

Comment 8

[poiru]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=fca263070a0c

https://hg.mozilla.org/integration/mozilla-inbound/rev/909655c3ec14

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/909655c3ec14

Comment 10

[poiru]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6fdb8eb0faac

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6fdb8eb0faac

Comment 13

[poiru]

Attachment: Fix indentation in xpcom/ds/

Comment 14

[poiru]

Attachment: Fix style violations in xpcom/ds/

Comment 15

[poiru]

Attachment: Fix style violations in xpcom/ds/

Fixed mistake in previous patch.

Comment 16

[Nathan Froyd [:froydnj]]

Comment on attachment 8451599 [details] [diff] [review]
Fix style violations in xpcom/ds/

Review of attachment 8451599 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, just a bunch of followup bugs to file...likely because this is old code.

::: xpcom/ds/StringBuilder.h
@@ +73,5 @@
>      free(mBuffer);
>    }
>  
>  private:
> +  char* mBuffer;

Followup bug to make this a mozilla::ScopedFreePtr and remove the destructor?

::: xpcom/ds/nsCharSeparatedTokenizer.h
@@ +189,5 @@
> +    , mSeparatorChar(aSeparatorChar)
> +    , mWhitespaceBeforeFirstToken(false)
> +    , mWhitespaceAfterCurrentToken(false)
> +    , mSeparatorAfterCurrentToken(false)
> +    , mSeparatorOptional(aFlags & SEPARATOR_OPTIONAL)

This is weird that we have duplicate classes like this; I wonder if anything keeps them from being templated appropriately.  File a followup bug?

::: xpcom/ds/nsSupportsArray.cpp
@@ +67,3 @@
>    }
>    printf("Max Size of SupportsArray:\n");
> +  for (i = 0; i < (int)(sizeof(MaxElements) / sizeof(MaxElements[0])); ++i) {

Feel free to move the declaration into these loops and/or make it unsigned.

::: xpcom/ds/nsSupportsPrimitives.cpp
@@ +426,3 @@
>    NS_ASSERTION(_retval, "Bad pointer");
>  
> +  char* result = (char*)nsMemory::Alloc(2 * sizeof(char));

Thank you!

::: xpcom/ds/nsVariant.cpp
@@ +539,5 @@
>  /***************************************************************************/
>  // These expand into full public methods...
>  
> +NUMERIC_CONVERSION_METHOD_NORMAL(VTYPE_INT8, uint8_t, Int8, (-127 - 1), 127)
> +NUMERIC_CONVERSION_METHOD_NORMAL(VTYPE_INT16, int16_t, Int16, (-32767 - 1), 32767)

Can you file a bug to make all of these use inttypes.h macros?

@@ +1681,5 @@
>  // For all the data getters we just forward to the static (and sharable)
>  // 'ConvertTo' functions.
>  
>  /* readonly attribute uint16_t dataType; */
> +NS_IMETHODIMP nsVariant::GetDataType(uint16_t* aDataType)

All the NS_IMETHODIMP in this file should be on separate lines.  If you're going to fix that up in another patch, that's fine.

::: xpcom/ds/nsWhitespaceTokenizer.h
@@ +92,5 @@
>    nsCWhitespaceTokenizerTemplate(const nsCSubstring& aSource)
> +    : mIter(aSource.Data(), aSource.Length())
> +    , mEnd(aSource.Data() + aSource.Length(), aSource.Data(), aSource.Length())
> +    , mWhitespaceBeforeFirstToken(false)
> +    , mWhitespaceAfterCurrentToken(false)

Please file a followup bug for these, too, similar to the CharTokenizer.h bug.

Comment 17

[poiru]

Attachment: Brace one-line control statements in xpcom/ds/

Comment 18

[poiru]

Attachment: Fix argument names in xpcom/ds/

(In reply to Nathan Froyd (:froydnj) from comment #16)
> Comment on attachment 8451599 [details] [diff] [review]
> Fix style violations in xpcom/ds/
> 
> Review of attachment 8451599 [details] [diff] [review]:
> -----------------------------------------------------------------
> 

Thank you for the quick reviews!

Comment 19

[Nathan Froyd [:froydnj]]

Comment on attachment 8452687 [details] [diff] [review]
Fix argument names in xpcom/ds/

Review of attachment 8452687 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the |aResult| change.

::: xpcom/ds/nsHashPropertyBag.cpp
@@ +12,5 @@
>  #include "nsVariant.h"
>  #include "mozilla/Attributes.h"
>  
>  nsresult
> +NS_NewHashPropertyBag(nsIWritablePropertyBag** aRetVal)

grep suggests that |aRetval| is slightly preferred, but |aResult| is more the norm.  Please use |aResult| here and throughout.

::: xpcom/ds/nsPersistentProperties.cpp
@@ +131,2 @@
>                                    // of this block
> +    nsAString& aOldValue);         // when duplicate property is found, new value

Please fix the comment alignment here.

@@ +223,5 @@
>              // but don't allow another '\n' to be skipped
>              mMultiLineCanSkipN = false;
>              // Now there is nothing to append to the mValue since we are skipping
>              // whitespaces at the beginning of the new line of the multiline
> +            // property. Set aTokenStart properly to ensure that nothing is appended

++ for fixing the comments, too!

Comment 20

[poiru]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=799ad1b16ac7

https://hg.mozilla.org/integration/mozilla-inbound/rev/57038b3a8c97

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/57038b3a8c97

Comment 22

[poiru]

Work will continue in bug 1046841.