check-style

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

check-style

Barnes, Peter D.
Hello Folks,

We’d like to run check-style on the entire code.  It’s been a while (have we ever done that?) and lot’s of cruft has crept in, mostly dangling white space.

It will be disruptive:  ~1140 out of ~2500 files with lots of small changes all over the place.

Any objections?  :)  Would holding off a week simplify you’re life tremendously?

Thanks,
Peter
_____________________________________________________________
Dr. Peter D. Barnes, Jr. CASC Division, B451 R2035
Lawrence Livermore National Laboratory Computation Directorate
7000 East Avenue, L-561 email:  [hidden email]
P. O. Box 808 Voice:  (925) 422-3384
Livermore, California 94550



Reply | Threaded
Open this post in threaded view
|

Re: check-style

Tom Henderson-2
On 4/8/20 7:28 PM, Barnes, Peter D. wrote:
> Hello Folks,
>
> We’d like to run check-style on the entire code.  It’s been a while (have we ever done that?) and lot’s of cruft has crept in, mostly dangling white space.

Yes, we did it 11 years ago, and vowed never to do it again :)

https://mailman.isi.edu/pipermail/ns-developers/2011-April/008654.html

>
> It will be disruptive:  ~1140 out of ~2500 files with lots of small changes all over the place.
>
> Any objections?  :)  Would holding off a week simplify you’re life tremendously?

The main issue IMO would be for people maintaining out-of-tree code.  
My guess is that it may impact LTE and Wi-Fi module users the most.

If we were to do it, I would wait until immediately after a release.  We
could also consider to exclude a module if desired.

I'd like to hear from others; it is not an urgent issue for me.

- Tom

Reply | Threaded
Open this post in threaded view
|

Re: check-style

Tommaso Pecorella
I'm totally up for this.

About out-of-tree code, I guess it would only impact the code that relies on diff-merges, and these are already impacted by the normal changes we do in the code.
As a result, I'd do it without second thoughts, even before a release.

Cheers,

T.

> On 9 Apr 2020, at 06:22, Tom Henderson <[hidden email]> wrote:
>
> On 4/8/20 7:28 PM, Barnes, Peter D. wrote:
>> Hello Folks,
>>
>> We’d like to run check-style on the entire code.  It’s been a while (have we ever done that?) and lot’s of cruft has crept in, mostly dangling white space.
>
> Yes, we did it 11 years ago, and vowed never to do it again :)
>
> https://mailman.isi.edu/pipermail/ns-developers/2011-April/008654.html
>
>>
>> It will be disruptive:  ~1140 out of ~2500 files with lots of small changes all over the place.
>>
>> Any objections?  :)  Would holding off a week simplify you’re life tremendously?
>
> The main issue IMO would be for people maintaining out-of-tree code.   My guess is that it may impact LTE and Wi-Fi module users the most.
>
> If we were to do it, I would wait until immediately after a release.  We could also consider to exclude a module if desired.
>
> I'd like to hear from others; it is not an urgent issue for me.
>
> - Tom
>

--------------------------------------------------------------

Who says nothing is impossible?
  Some people do it every day!
-- Alfred E. Neuman

--------------------------------------------------------------

Tommaso Pecorella - Ph.D.

Assistant professor
Dpt. Ingegneria dell'Informazione
Università di Firenze

CNIT - Università di Firenze Unit

via di S. Marta 3
50139, Firenze
ITALY

email: [hidden email]
       [hidden email]

phone : +39-055-2758540
mobile: +39-320-4379803
fax   : +39-055-2758570








Reply | Threaded
Open this post in threaded view
|

Re: check-style

Natale Patriciello
In reply to this post by Barnes, Peter D.
On 09/04/20 at 02:28am, Barnes, Peter D. wrote:
> Hello Folks,
>
> We’d like to run check-style on the entire code.  It’s been a while (have we ever done that?) and lot’s of cruft has crept in, mostly dangling white space.
>
> It will be disruptive:  ~1140 out of ~2500 files with lots of small changes all over the place.
>
> Any objections?  :)  Would holding off a week simplify you’re life tremendously?


Hi,

could we -at the same time- put in place some git commit style checks?
As well as moving our check_style script to something else than
uncrustify, that presents inconsistent styling across versions. I
remember that Alexander did some work with clang format, which I think
can be the ideal replacement.

Nat
Reply | Threaded
Open this post in threaded view
|

Re: check-style

Barnes, Peter D.
We can add check-style/uncrustify to the ns-3 CI scripts.

We're not able to take on implementing a different style checker.  Someone else will have to take that on.

Peter
_____________________________________________________________
Dr. Peter D. Barnes, Jr.                  CASC Division, B451 R2035
Lawrence Livermore National Laboratory    Computation Directorate
7000 East Avenue, L-561                   email:  [hidden email]
P. O. Box 808                             Voice:  (925) 422-3384
Livermore, California 94550
 

On 4/14/20, 12:14 AM, "Natale Patriciello" <[hidden email]> wrote:

    On 09/04/20 at 02:28am, Barnes, Peter D. wrote:
    > Hello Folks,
    >
    > We’d like to run check-style on the entire code.  It’s been a while (have we ever done that?) and lot’s of cruft has crept in, mostly dangling white space.
    >
    > It will be disruptive:  ~1140 out of ~2500 files with lots of small changes all over the place.
    >
    > Any objections?  :)  Would holding off a week simplify you’re life tremendously?
   
   
    Hi,
   
    could we -at the same time- put in place some git commit style checks?
    As well as moving our check_style script to something else than
    uncrustify, that presents inconsistent styling across versions. I
    remember that Alexander did some work with clang format, which I think
    can be the ideal replacement.
   
    Nat
   

Reply | Threaded
Open this post in threaded view
|

Re: check-style

Tom Henderson-2
In reply to this post by Natale Patriciello
On 4/14/20 12:14 AM, Natale Patriciello wrote:

> On 09/04/20 at 02:28am, Barnes, Peter D. wrote:
>> Hello Folks,
>>
>> We’d like to run check-style on the entire code.  It’s been a while (have we ever done that?) and lot’s of cruft has crept in, mostly dangling white space.
>>
>> It will be disruptive:  ~1140 out of ~2500 files with lots of small changes all over the place.
>>
>> Any objections?  :)  Would holding off a week simplify you’re life tremendously?
>
> Hi,
>
> could we -at the same time- put in place some git commit style checks?
> As well as moving our check_style script to something else than
> uncrustify, that presents inconsistent styling across versions. I
> remember that Alexander did some work with clang format, which I think
> can be the ideal replacement.


I would be up for this if we could make it work without adding too much
hassle to maintainers.  I assume that you mean a server-side hook on
GitLab.com that rejects the push.

The main reason that we did not implement this in years past with
Mercurial is that different versions of uncrustify were giving slightly
different output, so the server-side hook might reject the push despite
the submitter having a clean check-style.py result.  Will this require
maintainers to only install a specific version of uncrustify or
clang-format?  We can test current versions but I'm not sure if it
provides guarantees to be future-proof.

- Tom

Reply | Threaded
Open this post in threaded view
|

Re: check-style

Natale Patriciello
On 14/04/20 at 02:17pm, Tom Henderson wrote:

> On 4/14/20 12:14 AM, Natale Patriciello wrote:
> > Hi,
> >
> > could we -at the same time- put in place some git commit style checks?
> > As well as moving our check_style script to something else than
> > uncrustify, that presents inconsistent styling across versions. I
> > remember that Alexander did some work with clang format, which I think
> > can be the ideal replacement.
>
>
> I would be up for this if we could make it work without adding too much
> hassle to maintainers.  I assume that you mean a server-side hook on
> GitLab.com that rejects the push.

I think that we are not allowed to set server-side hooks with the public
gitlab instance; I was thinking on providing a set of scripts that can
be configured locally by the developer. They will not accept the commit,
if it doesn't "cleanly" pass the format style check.

> The main reason that we did not implement this in years past with Mercurial
> is that different versions of uncrustify were giving slightly different
> output, so the server-side hook might reject the push despite the submitter
> having a clean check-style.py result.  Will this require maintainers to only
> install a specific version of uncrustify or clang-format?  We can test
> current versions but I'm not sure if it provides guarantees to be
> future-proof.

I have googled a little, and seems that neither clang-format is
future-proof. That hurts a little, because it will be impossible to
make all the developers stick with a particular clang-format version,
especially when there is Apple involved in clang development :-(

Here [1] there is blog series in which the man in charge of implementing
this for MongoDB shared his experience. Some of interesting notes are
that they use a python script around clang-format, which download a
specific version of it [2]. Probably, we would have to do the same...

Nat


[1] https://engineering.mongodb.com/post/succeeding-with-clangformat-part-1-pitfalls-and-planning
[2] https://github.com/mongodb/mongo/blob/a751344c0b46fa7ea121fb74aa931797b783ef2b/buildscripts/clang_format.py
Reply | Threaded
Open this post in threaded view
|

Re: check-style

Barnes, Peter D.
On Apr 14, 2020, at 9:05 AM, Natale Patriciello <[hidden email]> wrote:

> Here [1] there is blog series in which the man in charge of
> implementing this for MongoDB shared his experience. Some of
> interesting notes are that they use a python script around
> clang-format, which download a specific version of it [2].
> Probably, we would have to do the same...

Or just mandate an uncrustify version...

P
 

Reply | Threaded
Open this post in threaded view
|

Re: check-style

Natale Patriciello
On 14/04/20 at 04:11pm, Barnes, Peter D. wrote:
> On Apr 14, 2020, at 9:05 AM, Natale Patriciello <[hidden email]> wrote:
>
> > Here [1] there is blog series in which the man in charge of
> > implementing this for MongoDB shared his experience. Some of
> > interesting notes are that they use a python script around
> > clang-format, which download a specific version of it [2].
> > Probably, we would have to do the same...
>
> Or just mandate an uncrustify version...

From my understanding, clang-format can be integrated in a wide variety
of IDEs, while uncrustify is completely manual.

Nat

Reply | Threaded
Open this post in threaded view
|

Re: check-style

Tom Henderson-2
In reply to this post by Barnes, Peter D.
On 4/14/20 6:47 AM, Barnes, Peter D. wrote:
> We can add check-style/uncrustify to the ns-3 CI scripts.
>
> We're not able to take on implementing a different style checker.  Someone else will have to take that on.

Peter, have you tried Alexander's script from here:
https://gitlab.com/nsnam/ns-3-dev/-/issues/2 and the .clang-format file
he put into the top-level directory?

- Tom

Reply | Threaded
Open this post in threaded view
|

Re: check-style

Tom Henderson-2
On 4/15/20 9:17 PM, Tom Henderson wrote:

> On 4/14/20 6:47 AM, Barnes, Peter D. wrote:
>> We can add check-style/uncrustify to the ns-3 CI scripts.
>>
>> We're not able to take on implementing a different style checker.  
>> Someone else will have to take that on.
>
> Peter, have you tried Alexander's script from here:
> https://gitlab.com/nsnam/ns-3-dev/-/issues/2 and the .clang-format file
> he put into the top-level directory?
>
> - Tom
>

I'm reviving this thread from April; can we try to resolve the next
steps now?

I think there are two issues:

1) (one time) whether we accept the breakage of off-tree code and clean
all accumulated style problems now

I tried uncrustify-0.59 on our codebase and committed the result here:
https://gitlab.com/tomhenderson/ns-3-dev/-/commit/2c0d310c90aaaf6a117597b30ef134cd7ef486a1

It is a large patch.  It also changes based on the uncrustify version;
Fedora 32's version (0.70), when re-run over the results of 0.59,
produced this patch:

https://gitlab.com/tomhenderson/ns-3-dev/-/commit/a73d764f4fe11150b6992cdb5f4d088dfb0bf8ae

My sense from the previous discussion was that maintainers are leaning
towards doing this, but it would be nice to hear now from any
maintainers who want to hold back.


2) (recurring) how to maintain-- whether to embed in our git workflow,
and whether to periodically rerun around release time if we are not
enforcing on a per-commit basis

Here is a brief discussion about how to possibly use clang-format:

https://www.electronjs.org/docs/development/clang-format

Alexander has also started the issue #2 cited above.

My thoughts are that we want to find a solution that minimizes hassle
for both maintainers and contributors, which would suggest that we would
have to mandate a specific version of the tool (uncrustify or
clang-format) for everyone, and provide some wrappers.

If someone has a more specific suggestion on how to do this, please
share.  Natale had shared some experience from MongoDB attempting the
same; should we try to replicate that (please see the earlier message
from April 14 regarding this).

- Tom
Reply | Threaded
Open this post in threaded view
|

Re: check-style

getachew.redieteab
Hi Tom,

> 1) (one time) whether we accept the breakage of off-tree code and clean
> all accumulated style problems now
>
> I tried uncrustify-0.59 on our codebase and committed the result here:
> https://gitlab.com/tomhenderson/ns-3-dev/-
> /commit/2c0d310c90aaaf6a117597b30ef134cd7ef486a1

I had a quick look at this and it is indeed very large, had to move to plain diff to be able to scroll down to the wifi module. Maybe it would be better to partition it per module (or group of modules) to be able to review the result a bit before pushing (all MRs at once though).
I'm suggesting this because there are quite some issues with letting uncrusitfy blindly do its work:

Example 1: Our dear maintainers' names have been affected... This is almost 10% of the changes of the wifi module. It would be better to replace the special characters before applying the script (or accept them in the rules).
- *          Matías Richart <[hidden email]>
- *          Sébastien Deronne <[hidden email]>
+ *          Matas Richart <[hidden email]>
+ *          Sbastien Deronne <[hidden email]>

Example 2: I find the proposed change a bit less readable that the original phrasing. Maybe the first alternative can also be put on a new line to make it easier to read while satisfying the style. So some reworking is necessary
diff --git a/src/wifi/model/channel-access-manager.cc b/src/wifi/model/channel-access-manager.cc
index 2592d67236c83fed7f708742d205a1d2346a2bf1..a4c04f4b51497751cdb5ac108f111d5ad209dac2 100644
--- a/src/wifi/model/channel-access-manager.cc
+++ b/src/wifi/model/channel-access-manager.cc
@@ -280,7 +278,7 @@ ChannelAccessManager::NeedBackoffUponAccess (Ptr<Txop> txop)
           // to correctly align the backoff start time at the next slot boundary
           // (performed by the next call to ChannelAccessManager::RequestAccess())
           Time delay = (txop->IsQosTxop () ? Seconds (0)
-                                           : m_sifs + txop->GetAifsn () * m_slot);
+                        : m_sifs + txop->GetAifsn () * m_slot);
           txop->UpdateBackoffSlotsNow (0, Simulator::Now () + delay);
         }
       else


>
> It is a large patch.  It also changes based on the uncrustify version;
> Fedora 32's version (0.70), when re-run over the results of 0.59,
> produced this patch:
>
> https://gitlab.com/tomhenderson/ns-3-dev/-
> /commit/a73d764f4fe11150b6992cdb5f4d088dfb0bf8ae

There are also some issues on this version:

Example 1: It's surprising that the last two lines get indented.
diff --git a/src/wifi/model/qos-txop.cc b/src/wifi/model/qos-txop.cc
index 2b8c820f78dc907cc3224b01415ca5c4648221e6..708799d56e96643103d115d639a16aa27209c0e8 100644
--- a/src/wifi/model/qos-txop.cc
+++ b/src/wifi/model/qos-txop.cc
@@ -257,16 +257,16 @@ QosTxop::PeekNextFrame (uint8_t tid, Mac48Address recipient)
 
   // lambda to peek the next frame
   auto peek = [this, &tid, &recipient, &it] (Ptr<WifiMacQueue> queue)
-  {
-    if (tid == 8 && recipient.IsBroadcast ())    // undefined TID and recipient
-      {
-        return queue->PeekFirstAvailable (m_qosBlockedDestinations, it);
-      }
-    return queue->PeekByTidAndAddress (tid, recipient, it);
-  }
-
-  // check if there is a packet in the BlockAckManager retransmit queue
-  it = peek (m_baManager->GetRetransmitQueue ());
+    {
+      if (tid == 8 && recipient.IsBroadcast ())  // undefined TID and recipient
+        {
+          return queue->PeekFirstAvailable (m_qosBlockedDestinations, it);
+        }
+      return queue->PeekByTidAndAddress (tid, recipient, it);
+    }
+
+    // check if there is a packet in the BlockAckManager retransmit queue
+    it = peek (m_baManager->GetRetransmitQueue ());
   // remove old packets
   while (it != m_baManager->GetRetransmitQueue ()->end () && IsQosOldPacket (*it))
     {

Example 2: this version of uncrustify sometimes rolled back on the previous version's changes
This version:
diff --git a/src/wifi/model/he-capabilities.cc b/src/wifi/model/he-capabilities.cc
index 41062a5e9a0dad36e6e10647904fda8b37663fa8..f03796d5595a7468a4e4ef209501d672871146ca 100644
--- a/src/wifi/model/he-capabilities.cc
+++ b/src/wifi/model/he-capabilities.cc
@@ -494,10 +494,10 @@ std::ostream &
 operator << (std::ostream &os, const HeCapabilities &HeCapabilities)
 {
   os << HeCapabilities.GetHeMacCapabilitiesInfo1 () << "|"
-  << +HeCapabilities.GetHeMacCapabilitiesInfo2 () << "|"
-  << HeCapabilities.GetHePhyCapabilitiesInfo1 () << "|"
-  << +HeCapabilities.GetHePhyCapabilitiesInfo2 () << "|"
-  << HeCapabilities.GetSupportedMcsAndNss ();
+     << +HeCapabilities.GetHeMacCapabilitiesInfo2 () << "|"
+     << HeCapabilities.GetHePhyCapabilitiesInfo1 () << "|"
+     << +HeCapabilities.GetHePhyCapabilitiesInfo2 () << "|"
+     << HeCapabilities.GetSupportedMcsAndNss ();
   return os;
 }
Previous version:
diff --git a/src/wifi/model/he-capabilities.cc b/src/wifi/model/he-capabilities.cc
index d89da00c9f1041a631bf3e8544295b98d629f24c..41062a5e9a0dad36e6e10647904fda8b37663fa8 100644
--- a/src/wifi/model/he-capabilities.cc
+++ b/src/wifi/model/he-capabilities.cc
@@ -494,10 +494,10 @@ std::ostream &
 operator << (std::ostream &os, const HeCapabilities &HeCapabilities)
 {
   os << HeCapabilities.GetHeMacCapabilitiesInfo1 () << "|"
-     << +HeCapabilities.GetHeMacCapabilitiesInfo2 () << "|"
-     << HeCapabilities.GetHePhyCapabilitiesInfo1 () << "|"
-     << +HeCapabilities.GetHePhyCapabilitiesInfo2 () << "|"
-     << HeCapabilities.GetSupportedMcsAndNss ();
+  << +HeCapabilities.GetHeMacCapabilitiesInfo2 () << "|"
+  << HeCapabilities.GetHePhyCapabilitiesInfo1 () << "|"
+  << +HeCapabilities.GetHePhyCapabilitiesInfo2 () << "|"
+  << HeCapabilities.GetSupportedMcsAndNss ();
   return os;
 }

>
> My sense from the previous discussion was that maintainers are leaning
> towards doing this, but it would be nice to hear now from any
> maintainers who want to hold back.

I'm clearly in favor, but we need to check the result a bit before pushing.

Best regards,

Rediet


_________________________________________________________________________________________________________________________

Ce message et ses pieces jointes peuvent contenir des informations confidentielles ou privilegiees et ne doivent donc
pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce message par erreur, veuillez le signaler
a l'expediteur et le detruire ainsi que les pieces jointes. Les messages electroniques etant susceptibles d'alteration,
Orange decline toute responsabilite si ce message a ete altere, deforme ou falsifie. Merci.

This message and its attachments may contain confidential or privileged information that may be protected by law;
they should not be distributed, used or copied without authorisation.
If you have received this email in error, please notify the sender and delete this message and its attachments.
As emails may be altered, Orange is not liable for messages that have been modified, changed or falsified.
Thank you.

Reply | Threaded
Open this post in threaded view
|

Re: check-style

Peter Barnes-4
In reply to this post by Tom Henderson-2
Hello Folks,

’m interested in this, of course, but won’t be able to follow up for two weeks.

I can’t really make out the diffs on my phone, so I can’t make any suggestions on the specific oddities mentioned in this thread,  but I’ll throw out a few thoughts anyway.

First, I think we should aim for a nearly completely automatic use of the formatter, otherwise we’ll be continually fighting it in the future. Unfortunately that means we’re going to have to scan a bunch of diffs and tweak the settings get it to do what we want. Fortunately we only have to this once-ish, until we settle on a config we’re willing to accept.

Second, I added NS_CHECKSTYLE_OFF, or something like that, to our config. I used it in a few places in core to preserve non-standard formatting which I thought helped make the code more readable. One place was a switch with very short cases. Putting each case on one line, extra space so things lined up, etc showed the parallelism that standard formatting would obscure. Of course finding those places requires scanning the diffs, but they only need to be protected once.

Finally, and I think the referenced article makes this point, part of the point of style fixing is to, well, fix the style. That’s going to mean each of us being willing to yield a bit on what we prefer, let the adopted style fixer do it’s job, and get back to writing good code, and arguing about const Foo &. :)

Peter

------------------
[hidden email]

> On Jun 29, 2020, at 10:00 AM, Tom Henderson <[hidden email]> wrote:
>
> On 4/15/20 9:17 PM, Tom Henderson wrote:
>>> On 4/14/20 6:47 AM, Barnes, Peter D. wrote:
>>> We can add check-style/uncrustify to the ns-3 CI scripts.
>>>
>>> We're not able to take on implementing a different style checker.  Someone else will have to take that on.
>> Peter, have you tried Alexander's script from here: https://gitlab.com/nsnam/ns-3-dev/-/issues/2 and the .clang-format file he put into the top-level directory?
>> - Tom
>
> I'm reviving this thread from April; can we try to resolve the next steps now?
>
> I think there are two issues:
>
> 1) (one time) whether we accept the breakage of off-tree code and clean all accumulated style problems now
>
> I tried uncrustify-0.59 on our codebase and committed the result here:
> https://gitlab.com/tomhenderson/ns-3-dev/-/commit/2c0d310c90aaaf6a117597b30ef134cd7ef486a1
>
> It is a large patch.  It also changes based on the uncrustify version; Fedora 32's version (0.70), when re-run over the results of 0.59, produced this patch:
>
> https://gitlab.com/tomhenderson/ns-3-dev/-/commit/a73d764f4fe11150b6992cdb5f4d088dfb0bf8ae
>
> My sense from the previous discussion was that maintainers are leaning towards doing this, but it would be nice to hear now from any maintainers who want to hold back.
>
>
> 2) (recurring) how to maintain-- whether to embed in our git workflow, and whether to periodically rerun around release time if we are not enforcing on a per-commit basis
>
> Here is a brief discussion about how to possibly use clang-format:
>
> https://www.electronjs.org/docs/development/clang-format
>
> Alexander has also started the issue #2 cited above.
>
> My thoughts are that we want to find a solution that minimizes hassle for both maintainers and contributors, which would suggest that we would have to mandate a specific version of the tool (uncrustify or clang-format) for everyone, and provide some wrappers.
>
> If someone has a more specific suggestion on how to do this, please share.  Natale had shared some experience from MongoDB attempting the same; should we try to replicate that (please see the earlier message from April 14 regarding this).
>
> - Tom