Open Bug 1243121 Opened 4 years ago Updated 13 days ago

C-C ldap directory: fix -Werror=sign-compare: signed vs unsigned warnings (now treated as errors)

Categories

(Thunderbird :: OS Integration, defect)

defect
Not set

Tracking

(Not tracked)

People

(Reporter: ishikawa, Assigned: ishikawa, NeedInfo)

References

Details

Attachments

(2 files, 8 obsolete files)

Compiler warning.
(unsigned >= 0) always.

Patch attached.

Well, we may actually want to make the value to "long long" instead of simple "long".

Someone in the know ought to ponder over this.

TIA
Assignee: nobody → ishikawa
Attachment #8712346 - Flags: review?(mkmelin+mozilla)
Attachment #8712346 - Flags: review?(mkmelin+mozilla) → review?(Pidgeot18)
Comment on attachment 8712346 [details] [diff] [review]
unsigned-vs-signed-in-sdk-memcache.patch

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

In the interest of not letting this just sit, I'll comment. If you would rather wait for jcranmer feel free to overrule my review and ask him again.

::: ldap/c-sdk/libraries/libldap/memcache.c
@@ +172,5 @@
>  /* Structure of a memcache object */
>  struct ldapmemcache {
>      unsigned long			ldmemc_ttl;
> +    long				ldmemc_size;
> +    long				ldmemc_size_used;

It seems to me that if your interest is in removing compile warnings, you should be specifically focused on that, rather than change this type which could have other effects.

So for example, where the code has:

if ( cache->ldmemc_size <= 0 ) {	/* no size limit */

change that to:

if ( cache->ldmemc_size == 0 ) {	/* no size limit */

as it is in some other cases. Also, remove:

assert(cache->ldmemc_size_used >= 0);

as this is meaningless.

If there is some specific behavior you are trying to fix, you should be specifying what the problem is.
Attachment #8712346 - Flags: review?(Pidgeot18) → review-
(In reply to Kent James (:rkent) from comment #1)
> Comment on attachment 8712346 [details] [diff] [review]
> unsigned-vs-signed-in-sdk-memcache.patch
> 
> Review of attachment 8712346 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> In the interest of not letting this just sit, I'll comment. If you would
> rather wait for jcranmer feel free to overrule my review and ask him again.
> 
> ::: ldap/c-sdk/libraries/libldap/memcache.c
> @@ +172,5 @@
> >  /* Structure of a memcache object */
> >  struct ldapmemcache {
> >      unsigned long			ldmemc_ttl;
> > +    long				ldmemc_size;
> > +    long				ldmemc_size_used;
> 
> It seems to me that if your interest is in removing compile warnings, you
> should be specifically focused on that, rather than change this type which
> could have other effects.
> 
> So for example, where the code has:
> 
> if ( cache->ldmemc_size <= 0 ) {	/* no size limit */
> 
> change that to:
> 
> if ( cache->ldmemc_size == 0 ) {	/* no size limit */
> 
> as it is in some other cases. Also, remove:
> 
> assert(cache->ldmemc_size_used >= 0);
> 
> as this is meaningless.
> 
> If there is some specific behavior you are trying to fix, you should be
> specifying what the problem is.

Thank you for your review.

I was trying to remove compiler warnings from under C-C portion of the compilation.

I was aware of the possibility of 32-bit signed vs unsigned range issue that may change the behavior as you correctly noted.
That is why I wondered if I may want to increase the width of the entity so that even if it becomes signed, 64-bit width comfortably can incorporate the 32-bit unsigned value. But then there may be a reason for the choice of the size, for example,
external specification requirement, etc. This is something I don't know much.

The reason I did not attack and modify the assertion() individually is that
unlike the bug Bug 1243117 where the issue seemed to be only in a couple of places and manual changes were not that difficult, the field member seems to be used in many places, and tracing its use was rather difficult.
A onetime change of type at the declaration looked an easier option.

Let me investigate a little more.
Although I do not use LDAP at home, but many enterprises do and
we should keep LDAP code as clean as possible.
[In that sense, the use of variadic macro for error reporting with variable # of
parameters will be more in demand than this patch :-( ]

TIA
Attached patch ldap-sign-compare.patch (obsolete) — Splinter Review
Here is an updated patch.
Actually this time, all the signed vs unsigned issues (warnings turned to errors by -Werror=sign-compare) are addressed.
But since it was deemed in 
Bug 1294260 - Fix Mailnews compiler warnings after they got upgraded to errors in bug 1292463 (Move MOZ_C{,XX}_SUPPORTS_WARNING to python configure) 
that ldap does not need to be handled at this moment by ignoring
-Werror=sign-compare, I am not asking for a review this time.
I am uploading the patch that I used to compile ldap directory with -Werror=sign-compare and some more in September.

There maybe a few more places where I handled additional issues such as
size of int vs long.

HOWEVER, I HAVE NOT TOUCHED the glaring problem, during 64-bit build, of 64-bit pointer first cast into 32-bit int and the two entities are subtracted and compared to an int (long?) value.
We should not cast the pointers to 32-bit value in 64-bit build.
I think this was the reason that I thought the hashing in ldap code does not work on 64-bit build. (The code was only looking at the lower 32-bit of 64-bit pointer, but I digress. So there is a very small chance that two items collides in a strange manner in 64-bit build.)


FYI.
Attachment #8712346 - Attachment is obsolete: true
Summary: Compiler warning: signed vs unsigned issue. (unsigned >= 0) always. ldap/c-sdk/libraries/libldap/memcache.c → C-C ldap directory: fix -Werror=sign-compare: signed vs unsigned warnings (now treated as errors)
See Also: → 1278795
See Also: → 1294260
Fix bitrot.

I did not touch the issues of 64-bit pointers cast into 32-bit ints and then subtracted. This may end up as undesired hash collision which *may* occur since the hash only looks at the lower 32-bit...
Attachment #8797114 - Attachment is obsolete: true
I know this is an external package and nobody wants to fix the issues concerning signed vs unsigned comparison, etc.

I threw in another fix.

FYI, I modified the long-standing issue of ber_len_t being defined as 32-bit entity even under 64-bit build (to produce 64-bit binary).
[I think this was what I thought would be a potential hash table bogus collision
when 64-bit binary was created: the difference of 64-bit pointer is compared only the 32-bit lower value only, thus we can possibly see an incorrect match or something.]
Latest GCC warns the casting of 64-bit pointer into 32 value at compile time.

Anyway, since it seems that this ber_len_t can be as large as 64 bit, I simply
set it to size_t and all is well as far as compilation goes AFTER a few casts
and outright incorrect prototypes size were used interchangeably and my change above uncovered them.

If someone is serious creating TB 64-bit version that uses LDAP extensively
in a corporate setup, this patch may help.

TIA
Attachment #8826661 - Attachment is obsolete: true
BTW,

for (i = 0 ; i < some_unsigned_entity; i++)

would produce GCC warning of signed vs unsigned comparison, and
should be cast as in

for (i = 0; (unsigned) i < some_unsigned_entity; i++)

since i >= 0 by the initializer 
provided that we won't overflow i as an integer, I think.

I noticed there is at least one instance of my patching it as follows, which 
may not be as appropriate as above:

for (i = 0; i < (int) some_unsigned_entity; i++)
 
CAVEAT EMPTOR.
> and outright incorrect prototypes size were used interchangeably and my change above uncovered them.

I meant to say
and outright incorrect prototypes were fixed.: incorrect size of the same size were used interchangeably and my change above uncovered them. Thus the additional fix.

Fixed bitrot by accommodating the clang-format changes of ldap directory.

One more patch which is posted in tandem is necessary to shut up format issues, etc. completely.

Attachment #8838476 - Attachment is obsolete: true

There are a few issues after the type changes, etc. in the format string.
This patch has to be applied in tandem to shut up format string issues found by GCC.

Both of these patches need to be applied AFTER the patch in
Bug 1277602
LDAP: Debug print macro LDAPDebug passes incorrect # of args to format strings and produced many WARNINGs at compile time.

Bitrot - Updated to the clang-formatted source tree.

Attachment #9077633 - Attachment is obsolete: true

Fixt bitrot due to clang-format change.

Attachment #9077634 - Attachment is obsolete: true

Just to check - this LDAP C-SDK in C-C is the canonical version, right? There's no upstream we should be tracking or anything?
(Looking at the headers, it looks like it's from the Netscape days, but LDAP is corporate enough that I could imagine one of the bigcorps picking it up and running with it :- )

Yeah, no upstream.

Comment on attachment 9090155 [details] [diff] [review]
fix clang-format BITROT: patch for printf format related issues in ldap source tree.

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

::: ldap/c-sdk/libraries/libldap/tmplout.c
@@ +617,5 @@
>            }
>            if (html) {
>              sprintf(buf, "<DD>%s<BR>%s", p, eol);
>            } else {
> +            snprintf( buf, 4095, "%-*s%s%s", labelwidth, " ", p, eol );

Why are these leading spaces added in some cases?

Yes, this was an external library but it seems we do not plan to upgrade it (if there is no upstream) and do not maintain it much.
But if you want to fix the compiler warnings and can do so without introducing new bugs, we can surely accept the patch. Thanks.

Comment on attachment 9090154 [details] [diff] [review]
clang-format change bitrot: ldap-sign-compaldap-sign-compare.patch (with ber_len_t change, too)

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

::: ldap/c-sdk/include/ldap-extension.h
@@ +811,5 @@
>  #define LDAP_UTF8COPY(d, s) \
>    ((0x80 & *(unsigned char *)(s)) ? ldap_utf8copy(d, s) : ((*(d) = *(s)), 1))
> +
> +#define LDAP_UTF8GETCC(s) ((0x80 & *(unsigned char*)(s)) ? ldap_utf8getcc (&s) : (unsigned long) *s++)
> +#define LDAP_UTF8GETC(s) ((0x80 & *(unsigned char*)(s)) ? ldap_utf8getcc ((const char**)&s) : (unsigned long) *s++)

This is getting a bit long and will probably not pass by our code reformatter.

::: ldap/c-sdk/libraries/libldap/getoption.c
@@ +415,5 @@
>        aip->ldapai_vendor_name = NULL;
>        return (LDAP_NO_MEMORY);
>      }
>  
> +    for (i = 0; (unsigned) i < NSLDAPI_EXTENSIONS_COUNT; ++i) {

At some for()s you seem to add some more indent (maybe tabs?). Please check those.

(In reply to :aceman from comment #14)

Comment on attachment 9090155 [details] [diff] [review]
fix clang-format BITROT: patch for printf format related issues in ldap
source tree.

Review of attachment 9090155 [details] [diff] [review]:

::: ldap/c-sdk/libraries/libldap/tmplout.c
@@ +617,5 @@

       }
       if (html) {
         sprintf(buf, "<DD>%s<BR>%s", p, eol);
       } else {
  •        snprintf( buf, 4095, "%-*s%s%s", labelwidth, " ", p, eol );
    

Why are these leading spaces added in some cases?

This could be tab vs untabify (tabs converted to space issues.)
I hope my running the code via local formatter to recreate the patch would help.

(In reply to :aceman from comment #15)

Yes, this was an external library but it seems we do not plan to upgrade it (if there is no upstream) and do not maintain it much.
But if you want to fix the compiler warnings and can do so without introducing new bugs, we can surely accept the patch. Thanks.

As far as my testing goes locally and on tryserver, we don't see additional errors. (I wonder how much testing LDAP gets, though.)

Anyway, as I mentioned elsewhere, this set of patches are applied in my tryserver runs for sometime and I don't see any errors from the patch set.
See, for example, https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=545674bbbce29f7c6e7b92b63e277a76ac5ceb68
The patches have been thrown in to my tryserver runs since mid July or sometime around it. (I had to recreate patch to apply to the latest clang-formatted C-C tree anyway.)

(In reply to :aceman from comment #16)

Comment on attachment 9090154 [details] [diff] [review]
clang-format change bitrot: ldap-sign-compaldap-sign-compare.patch (with
ber_len_t change, too)

Review of attachment 9090154 [details] [diff] [review]:

::: ldap/c-sdk/include/ldap-extension.h
@@ +811,5 @@

#define LDAP_UTF8COPY(d, s)
((0x80 & *(unsigned char )(s)) ? ldap_utf8copy(d, s) : (((d) = (s)), 1))
+
+#define LDAP_UTF8GETCC(s) ((0x80 & (unsigned char)(s)) ? ldap_utf8getcc (&s) : (unsigned long) s++)
+#define LDAP_UTF8GETC(s) ((0x80 & (unsigned char)(s)) ? ldap_utf8getcc ((const char
)&s) : (unsigned long) *s++)

This is getting a bit long and will probably not pass by our code
reformatter.

So I think I would rather let the formatter handle the accepted line break, etc.
I don't have any preference here EXCEPT that I kept the original line fomratting as much as possible and
try to mimic the original line formatting if I needed to create a new line similar to the existing ones.

So my idea is to get the review for logical changes, and then modified the code accordingly LOGICALLY speaking, and then
run the formatter locally to produce a new patch to conform to the formatter's idea of proper formatting.

What do you think?
If I run the formatter before the full review, we may lose a bit of ease of comparison against the old copy.
But given the tool for comparing ("diff") on bugzilla, I have to agree this is moot.

::: ldap/c-sdk/libraries/libldap/getoption.c
@@ +415,5 @@

   aip->ldapai_vendor_name = NULL;
   return (LDAP_NO_MEMORY);
 }
  • for (i = 0; (unsigned) i < NSLDAPI_EXTENSIONS_COUNT; ++i) {

At some for()s you seem to add some more indent (maybe tabs?). Please check
those.

So I can run cflow-format locally and produce a new set of patches without touch the code at all otherise.

What is preferable?

(In reply to ISHIKAWA, Chiaki from comment #17)

  •        snprintf( buf, 4095, "%-*s%s%s", labelwidth, " ", p, eol );
    

Why are these leading spaces added in some cases?
This could be tab vs untabify (tabs converted to space issues.)
I hope my running the code via local formatter to recreate the patch would help.

I mean the space before "buf". Surely it is not any formatter adding those?

I'm not sure we indend to run the C++ clang formatter on the ldap folder anytime soon (maybe also because this seems to be pure C?) so I think we should do any line wraps manually in this patch.

The leading spaces before for() are here:
--- a/ldap/c-sdk/libraries/libprldap/ldappr-io.c
+++ b/ldap/c-sdk/libraries/libprldap/ldappr-io.c
@@ -261,33 +261,33 @@ prldap_poll(LDAP_X_PollFD fds[], int nfd

  •  for (j = 0; j < PRLDAP_EVENTMAP_ENTRIES; ++j) {
    
  •        for (j = 0; (unsigned) j < PRLDAP_EVENTMAP_ENTRIES; ++j) {
    

We can surely drop these from the patch manually, no need to run any reformatter and get many more lines touched.

Formatting has happened:
https://hg.mozilla.org/comm-central/rev/0c50a354ca18
Reformat to Google coding style in ldap/. rs=reformat
https://hg.mozilla.org/comm-central/rev/96a805927f55
replace tabs with spaces in ldap/ C files and fix comments badly formatted by reformatting. rs=white-space-only
https://hg.mozilla.org/comm-central/rev/dc3b0d8e7139
Repeat running clang-format on ldap/ and mailnews/mime. rs=reformat

(In reply to :aceman from comment #20)

(In reply to ISHIKAWA, Chiaki from comment #17)

  •        snprintf( buf, 4095, "%-*s%s%s", labelwidth, " ", p, eol );
    

Why are these leading spaces added in some cases?
This could be tab vs untabify (tabs converted to space issues.)
I hope my running the code via local formatter to recreate the patch would help.

I mean the space before "buf". Surely it is not any formatter adding those?

Oh, I think that is me :-)

(In reply to :aceman from comment #21)

I'm not sure we indend to run the C++ clang formatter on the ldap folder anytime soon (maybe also because this seems to be pure C?) so I think we should do any line wraps manually in this patch.

The leading spaces before for() are here:
--- a/ldap/c-sdk/libraries/libprldap/ldappr-io.c
+++ b/ldap/c-sdk/libraries/libprldap/ldappr-io.c
@@ -261,33 +261,33 @@ prldap_poll(LDAP_X_PollFD fds[], int nfd

  •  for (j = 0; j < PRLDAP_EVENTMAP_ENTRIES; ++j) {
    
  •        for (j = 0; (unsigned) j < PRLDAP_EVENTMAP_ENTRIES; ++j) {
    

We can surely drop these from the patch manually, no need to run any reformatter and get many more lines touched.

I think we can run formatter now as Jorg pointed out in comment 22.
I would prefer to have the formatter to fix my whitespace faux pa if possible.
But there may be fine print.

(In reply to Jorg K (GMT+2) from comment #22)

Formatting has happened:
https://hg.mozilla.org/comm-central/rev/0c50a354ca18
Reformat to Google coding style in ldap/. rs=reformat
https://hg.mozilla.org/comm-central/rev/96a805927f55
replace tabs with spaces in ldap/ C files and fix comments badly formatted by reformatting. rs=white-space-only
https://hg.mozilla.org/comm-central/rev/dc3b0d8e7139
Repeat running clang-format on ldap/ and mailnews/mime. rs=reformat

Jorg, one thing I need to know.
There seems to be some type of configuration file to control the fomrating.
The necessary file for formating ldap directory (and the subdirectories below) is in the hg repository and
so in my local copy, too, correct?

OR is there a fine print warning?
As you mentioned in your comment above, you took care of whitespace issues separately.

replace tabs with spaces in ldap/ C files and fix comments badly formatted by reformatting. rs=white-space-only
DO WE HAVE TO HANDLE TAB and SPACE specially by hand?!
That is if I run cflow-format again, it will mess up your tabs vs. whitespaces change?

If the above is cleared up, I take that based on Jorg's post to Maildev mailing list on April 23,
Subject: [Maildev] Reformatting the comm-central C++ code base to Google coding style

If you want to try it out: mach clang-format -p comm/dir/of/your/choice, issued from the M-C top source directory.

I can run the above command for
the following directories AFTER I apply each patch, and obtain the new patch that contains the formatting approved by the official formatting
including space/tab issues.
(And repeat this for each patch.)

comm/ldap/c-sdk/include
comm/ldap/c-sdk/libraries/libprldap
comm/ldap/c-sdk/libraries/liblber
comm/ldap/c-sdk/libraries/libldif
comm/ldap/c-sdk/libraries/libldap

Correct?

I was a bit hesitant to do this yet since I still use "hg MQ" extension to create patches.
This MQ usage is now officially deprecated or abandoned style of hg usage for source management.
I realized this after the following problem: the venerable |mach bootstrap| command, if I am not carefull enough,
created .hg file that breaks MQ usage.
A couple of months or so ago, for experiment, I turned on automatic formatting in checking when |mach bootstrap|
suggested to turn on this feature. It touches and modifies .hg file.
Unfortunately, this feature messed up MQ patches.
(For whatever the reason, the automatic editing feature for message (-m) of "hg MQ" qnew or qrefr started to be invoked TWICE after I enabled automatic formatting, and by the time the second editor invocation happened, the original patch content was erased or something...
So I disabled the automatic formatting function in check-in, etc. for now.

Of course, I can manually run the above hopefully without messing up the source tree, I hope.
As long as the necessary configuration file is there, I should be OK even for tabs vs whitespaces issue.

No?

TIA

I forgot to mention.
For the second part of the patch set,

fix clang-format BITROT: patch for printf format related issues in ldap source tree.

I throw in the use of snprintf instead of the original sprintf since there were some dubious printing that may really goes close to the string size limit.
However, generally speaking, I think the use of original sprintf was rather safe, but why NOT accept the warning and modify the code to use safer coding practice?
There were cases when I manually guessed the size of a scalar value and number of digits or characters necessary for printing, and if my guessed range was correct, the use of sprintf without bound checking seemed OK. But such checking should be left to the computer.

The file that configures the formatting is in the tree, .clang-format, at the top level.

You can run mach clang-format -p comm/ldap/c-sdk/include, or in subdirectories then refresh your patch.

clang-format sadly don't replace tabs, but unless I missed something, there shouldn't be any tabs anywhere in the source tree any more.

mach bootstrap may refresh your Mercurial version. I use it from time to time, I also use HG queues, and I haven't seen any problem.

I hope I've answered everything. Please ask precise questions, that's easier then to scan a large block of text for questions.

Dear Jorg,

Thank you for the answers.

(In reply to Jorg K (GMT+2) from comment #25)

The file that configures the formatting is in the tree, .clang-format, at the top level.

You can run mach clang-format -p comm/ldap/c-sdk/include, or in subdirectories then refresh your patch.

I am a bit surprised that mach clang-format -p comm/ldap/c-sdk/include does not seem to touch the source tree on my PC at all.
after I applied my local patches.
I am afraid that |mach clang-format| is not working on my PC...
I don't think my hand-created patch conforms to c++ style completely.
(For example, I had a space " " after "(", but it did not complain. Maybe it is allowed by clang-format.)

clang-format sadly don't replace tabs, but unless I missed something, there shouldn't be any tabs anywhere in the source tree any more.

I will check my patch to see if I introduced any tabs or something.
There is one instance where the whitespace indentation at the beginning of a line is messed up.

mach bootstrap may refresh your Mercurial version. I use it from time to time, I also use HG queues, and I haven't seen any problem.

|mach bootstrap| is OK as long as you DON'T ENABLE automatic clang-formatting at HG commit (even locally).
If you do, somehow the editor that you have specified for editing the message (-m) for qrefr or qnew gets invoked TWICE IN SUCCESSION and
the second time, the editor is invoked your patch becomes an empty patch :-(

I hope I've answered everything. Please ask precise questions, that's easier then to scan a large block of text for questions.

Thank you.

I am afaaid that somehow |mach clang-format| is not working on my PC.
I disabled the "automatic" formatting on check-in feature suggested by |mach bootstrap| manually by commenting out a couple of lines from .hgrc. That may have something to do the apparent non-invocation.
OR my hand-crafted code conforms to google style completely(?!).
Even the whitespace issues mentioned by aceman in comment 21 is not touched by |mach clang-format|? (Again this is unlikely so that makes me think that |mach clang-format| is a noop on my PC at this moment.)

I tried to see if enabling the clang-format, js-format extensions in .hgrc would enable |clang-format| to correctly.
No I don't think so.
And trying that I think I messed my local source tree. Definitely enabling the formatting on commit (locally) even in the form of
hg MQ is fatal.
I will recreate my source tree over the weekend.

I have no idea how to go about this |mach clang-format| issue.
Is it possible that someone can apply the patches and then re-post the formatted patch on bugzilla?

TIA

If it's not working, you probably have some odd modification in some configuration. Do these, and then see if things start working, and make sure you use the correct paths

hg qpop --all
hg purge
hg pull -u && hg up default
cd ..
./mach bootstrap

(In reply to Magnus Melin [:mkmelin] from comment #28)

If it's not working, you probably have some odd modification in some configuration. Do these, and then see if things start working, and make sure you use the correct paths

hg qpop --all
hg purge
hg pull -u && hg up default
cd ..
./mach bootstrap

Thanks, I will report my findings over the weekend.

(In reply to ISHIKAWA, Chiaki from comment #29)

(In reply to Magnus Melin [:mkmelin] from comment #28)

If it's not working, you probably have some odd modification in some configuration. Do these, and then see if things start working, and make sure you use the correct paths

hg qpop --all
hg purge
hg pull -u && hg up default
cd ..
./mach bootstrap

Thanks, I will report my findings over the weekend.

I am sorry that the above did not quite worked well.

Do people use the hg's feature of enabling us to edit, using an editor, the message when we do NOT specify "-m messagestring" to the following commands?
hg qnew
hg qrefersh

I do and hg invokes a specified editor (in my case, it is emacs usually, but I tested the operation with vi as well).
When I enable clang-format on check-in, the editor is invoked TWICE and trashed the patch.

HOWEVER, I think if I specify "-m messagestring" to the above commands, and do NOT invoke the editor as a result, I do not trash patch at all.
This I recall with 20/20 hindsight.

BTW, will some kind soul send me the working ~/.hgrc config file with which you are certain that |mach clang-format ...| works?
I have manually edited .hgrc (and obviously disabled a couple of lines related to clang-format upon checkin due to the problems I described.).
Comparision with working ~/.hgrc may help.

TIA

PS: In any case, I will create a small test directory under ./mozilla and
see if |mach clang-format ...| works on THAT subdirectory instead of "./comm".
I don't want to trash my real patches any more while I am testing the behavior of |mach clang-format ...|.

I have finally figured out at least the cause of the failure to run |mach clang-format -p pathname| successfully.

I have stored mozconfig under a non-default name of mozconfig-tb3 and set up various environment variables with which to run mozilla development tools.

I found out that I really needed to run my own script |./do-make-tb.sh clang-format -p pathname| to run |mach clang-format -p pathname| AFTER setting up these environment variables and such.
It would have been great if the bare |mach clang-format -p pathname| would complain about missing commands in the PATHs, or missing mozconfig, whatever, but it did not loudly and so I didn't realize this.

Now I have verified that clang-format did kick in to format my test source file under a test directory I created.

I still have no idea about why I expereince the multiple invocation of editor after enabling clang-format when one uses the editor to fill in the message for | hg qnew| or |hg qrefr|, but I have to be careful for now. | hg qnew -m message| or |hg qrefr -m message| does not invoke an editor and I don't trash patches even if I use MQ.

Once I run the LDAP patches through clang-format, I will re-upload the patches That should contain the correct format style after all and I hope the whitespace issues are taken care of automatically
.

TIA

Just to note, there is an issue with ./mach clang-format not picking up the formatting rules in comm/.clang-format under Linux (seems ok on Windows). Notes and workaround in Bug 1575449.

(In reply to Ben Campbell from comment #32)

Just to note, there is an issue with ./mach clang-format not picking up the formatting rules in comm/.clang-format under Linux (seems ok on Windows). Notes and workaround in Bug 1575449.

Thank you for heads-up. Yeah, I am using linux. I have to figure out where the tmp directory used by mach exactly.
I am specifying TMP, TMPDIR explicitly and so that may be where I should copy the .clang-format file.

Thank you again.

Now clang-formatted.
This should take care of my gratuitous handling of whitespace.

This builds locally and on tryserver.
See, e.g., https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8b1bbba78ca71568446d58bad2327ddb2dafda25

Attachment #9090154 - Attachment is obsolete: true

Now clang-formatted.
This should take care of my gratuitous handling of whitespace.

This builds locally and on tryserver.
See, e.g., https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8b1bbba78ca71568446d58bad2327ddb2dafda25

Attachment #9090155 - Attachment is obsolete: true

I am a bit put off by clang-format putting type cast without a space before an expression.
That is, I prefer
(int) x
rather than
(int)x
but maybe it is only me.

Aceman,

The patches have been clang-formatted.

From cursory reading, I believe all the whitespace issues that you noticed have been taken care of by clang-format.
Well, you and I may not some manners in which formatting happens, but that is life :-)

Can you kindly take a look at this?

Flags: needinfo?(acelists)
You need to log in before you can comment on or make changes to this bug.