clang-format: add styles and reformat ifcparse files #3761

dirkolbrich · 2023-09-15T13:20:46Z

This is a first test setting with clang-format and a complete reformat of the /src/ifcparse folder except generated files.

dirkolbrich · 2023-09-15T13:21:38Z

Please look through the changes in formatting and comment, if this would work for the overall project and C++ code.

All code changes are done by clang-format, except:

removed unnecessary ../ifcparse/ path in include statements, if the included file is in the same folder
added license text if missing from file

dirkolbrich · 2023-09-15T13:27:11Z

src/ifcparse/Argument.h

+    virtual operator std::vector<std::vector<int>>() const;
+    virtual operator std::vector<std::vector<double>>() const;
+    virtual
+    operator boost::shared_ptr<aggregate_of_aggregate_of_instance>() const;


line break prop. due to statement line length

virtual operator boost::shared_ptr<aggregate_of_aggregate_of_instance>() const; is +- 80 chars it seems. Can we increase that?

LLVM and Google style use a ColumnLimit of 80. We could set it to 120, but there will always be a line which is just 1-2 characters too long. Or we could NOT introduce a limit for the beginning (setting it to 0) and bet on some form of self-control?

I would choose either 120 or 0. You decide.

dirkolbrich · 2023-09-15T13:27:58Z

src/ifcparse/ArgumentType.h

 #include "ifc_parse_api.h"
+#include "IfcSchema.h"


These need to go after the include guard.

agreed. not sure what happened here, maybe some merge artefact, or just a weird oversight.

dirkolbrich · 2023-09-15T13:39:11Z

src/ifcparse/IfcSIPrefix.cpp

+    else if (v == "ATTO")
+        return 1.e-18;
+    else
+        return 1.;
 }



to prevent a break after every else if ()this block needs to be commented with // clang-format off. Probably needs to tweak the setting AllowShortIfStatementsOnASingleLine.

For me it's not an issue. This kind of formatting/alignment for readability tends to be a pain to maintain anyway. For me it's not necessary to preserve it.

I wonder though. Shouldn't we require braces around every if-statement/for-loop/etc. body? It's so easy to introduce errors (granted indentation would help you, but still):

if (something) do_something(); do_something_else();

There is an option to InsertBraces, but clang advises on extra caution to set this https://clang.llvm.org/docs/ClangFormatStyleOptions.html#insertbraces. I think the first format with this option could lead to falsely introduced braces, the following format runs should be fine.

I would say let's give it a try. I don't know which kind of situations could give issues. I'd imagine they would result into compilation errors due to changes in scope, but yeah if you're unlucky maybe it results into a subtle behaviour changes. I can only think macro-based loops such as BOOST_FOREACH that would depend on more context than clang-format has, but that would then be a case of too little braces. I will review carefully.

dirkolbrich · 2023-09-15T13:41:06Z

src/ifcparse/IfcSpfHeader.cpp

+static const char* const FILE_NAME = "FILE_NAME";
+static const char* const FILE_SCHEMA = "FILE_SCHEMA";
+static const char* const ENDSEC = "ENDSEC";
+static const char* const DATA = "DATA";
 // The following header entities are not normally encountered in IFC files and are not parsed.


needs to tweak AlignConsecutiveAssignments and its options

Again, not a big issue for me. You already see in the previous state it got mangled between tab width and spaces. Besides https://clang.llvm.org/docs/ClangFormatStyleOptions.html#alignconsecutiveassignments does a lot of things I wouldn't consider to improve readability. Only in these cases where you're basically defining aliases it makes some sense to me, not in the general case of assignments. But I don't think clang-format will be able to meaningfully differentiate between those cases.

Ok, we can leave this out for the start.

dirkolbrich · 2023-09-15T13:42:54Z

src/ifcparse/IfcWrite.h

+        std::vector<std::vector<double>>,
+        // An aggregate of an aggregate of entities. E.g. ((#1, #2), (#3))
+        aggregate_of_aggregate_of_instance::ptr>
+        container;


here the closing right angle of boost::Variant<> should break to the next line to get a better readability. have not found the right setting yet.

This is really an exception though. I wouldn't spend too much time fighting with this.

aothms · 2023-09-16T11:26:41Z

.clang-format

+AllowShortBlocksOnASingleLine: Always
+AllowShortEnumsOnASingleLine: false
+AllowShortIfStatementsOnASingleLine: OnlyFirstIf


I would prefer never. Just for consistency.

Suggested change

AllowShortBlocksOnASingleLine: Always

AllowShortEnumsOnASingleLine: false

AllowShortIfStatementsOnASingleLine: OnlyFirstIf

AllowShortBlocksOnASingleLine: Never

AllowShortEnumsOnASingleLine: false

AllowShortIfStatementsOnASingleLine: Never

aothms · 2023-09-16T11:32:00Z

src/ifcparse/IfcLogger.h

-	static void PrintPerformanceStats();
-	static void PrintPerformanceStatsOnElement(bool b) { print_perf_stats_on_element = b; }
+  public:
+    typedef enum {


I guess this doesn't need to be a typedef. Is there a rule for that. Not sure if we use it often.

aothms · 2023-09-16T11:34:29Z

This is excellent. Thanks for highlighting some things. I'm really happy that it doesn't indent after namespaces. That's such a relief.

removed unnecessary ../ifcparse/ path in include statements, if the included file is in the same folder

This was on purpose. To make it discoverable which module you're importing from. I must admit it's not incredibly useful in ifcopenshell. But I've seen codebases where it really helps.

One thing that I naively hoped, but which doesn't seem possible to to reformat case and postfix. In general I prefer this convention:

class IFC_PARSE_API base_class : public virtual base_interface {
  private:
    uint32_t identity_;
    static std::atomic_uint32_t counter_;

So lowercase with underscore. Private/protected members get a postfix additional underscore.

It seems it can be checked, but not formatted:

https://clang.llvm.org/extra/clang-tidy/checks/readability/identifier-naming.html#readability-identifier-naming

dirkolbrich · 2023-09-16T12:43:20Z

Yeah, this would be `ColumnLimit` with a sensible default of 120? phonephone

aothms · 2023-09-16T12:59:37Z

120 sounds good to me

dirkolbrich · 2023-09-16T16:05:16Z

I'm really happy that it doesn't indent after namespaces. That's such a relief.

If you really insist that much, we could turn this on via NamespaceIndentation 😜

In general I prefer this convention:
class IFC_PARSE_API base_class : public virtual base_interface {
  private:
    uint32_t identity_;
    static std::atomic_uint32_t counter_;
Private/protected members get a postfix additional underscore.

No, there is no option to format that in. I will check if we can setup clang-tidy to give a warning during linting and recommend the preferred style. Usually the type of naming convention should be documented in a style guide like Google does it and enforced during code review.

Lower case with underscore for function or variable names is fine with me (despite I personally prefer camelCase). But generally here I disagree with the use of a trailing underscore for private members variable names. This tends to get visually lost, especially if the declaration or use is within a larger codeblock. Better to explicitly name a private member with a prefix like m_private_member.

For type names like Classes I personally prefer PascalCase like Google does it as this differentiates a type from a function visually more clearly.

aothms · 2023-09-16T16:38:49Z

Better to explicitly name a private member with a prefix like m_private_member.

I really prefer a postfix due to typing speed with code completion. Any convention you're aware of that'd be a good compromise?

The lower_case vs camelCase thing I can adapt to in either way. I personally prefer lower case due to similarity with boost / stl / most of python, but my intuition says actually camelCase is preferred among most. I do want more consistency between C++ and Python classes (less renames in swig) so this might turn out to be backwards incompatible on the python side where we have the most use.

dirkolbrich · 2023-09-17T10:10:41Z

Ok, will leave the naming conventions for variables and type definitions as is.

dirkolbrich · 2023-09-17T10:52:33Z

Next format test with minor changes to the clang-format style. The most notable not enforcing column length. the formatting looks good to me.

dirkolbrich · 2023-09-17T10:55:45Z

removed unnecessary ../ifcparse/ path in include statements, if the included file is in the same folder

This was on purpose. To make it discoverable which module you're importing from. I must admit it's not incredibly useful in ifcopenshell. But I've seen codebases where it really helps.

This should not be a readability problem any more, as the formatting now groups include blocks with the same folder includes as the first block, see the file IfcParse.h for example:

#ifndef IFCPARSE_H
#define IFCPARSE_H

#if defined(IFCOPENSHELL_BRANCH) && defined(IFCOPENSHELL_COMMIT)
#define IFCOPENSHELL_VERSION STRINGIFY(IFCOPENSHELL_BRANCH) "-" STRINGIFY(IFCOPENSHELL_COMMIT)
#else
#define IFCOPENSHELL_VERSION "0.8.0"
#endif

#include "aggregate_of_instance.h"
#include "Argument.h"
#include "ifc_parse_api.h"
#include "IfcBaseClass.h"
#include "IfcCharacterDecoder.h"
#include "IfcLogger.h"
#include "IfcSpfStream.h"
#include "macros.h"

#include <boost/dynamic_bitset.hpp>
#include <boost/shared_ptr.hpp>
#include <cstring>
#include <fstream>
#include <iostream>
#include <map>
#include <sstream>
#include <string>
#include <vector>

namespace IfcParse {

aothms · 2023-09-17T11:01:47Z

It looks good to me as well. Very nice :) Shall we merge?

dirkolbrich · 2023-09-17T11:04:04Z

Let it sit for a minute. Maybe I see some things during my Sunday coffee and cake session, like duplicate clang-format setting, which are already in the derived base style.

dirkolbrich · 2023-09-18T14:16:29Z

I fixed some alignments of license blocks and shortened some exec line length. I think I'm good to merge.

dirkolbrich · 2023-09-25T08:50:50Z

@aothms I think we can merge, if you agree.

dirkolbrich · 2023-10-02T19:02:26Z

@aothms Can we move forward with this PR or are there any modifications needed?

- split up some very long lines

dirkolbrich · 2023-10-09T09:44:35Z

@aothms Here is my weekly reminder for this PR. Can we move forward with the integration of clang-format. Or are there any changes needed on this? I'm happy contribute.

Krande · 2023-10-12T10:27:38Z

@aothms Can this be merged or will it interfere with #3826 or #3827 (I haven't checked it myself, but can do so if you want)?

@dirkolbrich I can merge this if @aothms gives us the thumbs up :)

dirkolbrich · 2023-10-12T10:33:00Z

There shouldn't be any conflicts, as PR #3826 and #3827 only touch src/ifcgeom code. This PR modifies src/ifcparse exclusively, but only in code style and formatting, no functional modifications.

aothms · 2023-10-12T10:39:12Z

Sorry, for the delay. I've been flooded with other things. Thanks again!

aothms · 2023-10-13T08:17:47Z

FYI I get this with my local MSVC 2017 build:

Which seems to be because of an outdated clang-format v6 that's shipped with msvc 2017

https://developercommunity.visualstudio.com/t/clang-format-incorrect-options/683472

The following changes seem to work with msvc 2017. But I must say I'm not impressed in general with the clang-format integration in msvc2017. This is not an issue though. For v0.8 I'll move to msvc 2019/2022 because anyway also Qt6 requires it, which we now depend on for the C++ ifc viewer.

I'm just leaving this here in case somebody stumbles upon this but for now I don't see this as an issue. It would be good to pin a clang-format version in the future though.

diff --git a/.clang-format b/.clang-format
index 40a0f424d..e0b190914 100644
--- a/.clang-format
+++ b/.clang-format
@@ -5,7 +5,7 @@ IndentWidth: 4
 ---
 Language: Cpp
 AllowShortEnumsOnASingleLine: false
-AlwaysBreakTemplateDeclarations: Yes
+AlwaysBreakTemplateDeclarations: true
 BinPackArguments: false
 BinPackParameters: false
 BraceWrapping:
@@ -14,11 +14,10 @@ BraceWrapping:
     SplitEmptyNamespace: false
 IncludeBlocks: Regroup
 InsertBraces: true
-InsertNewlineAtEOF: true
 PackConstructorInitializers: CurrentLine
 PointerAlignment: Left
 ReflowComments: false # keep licenze comment blocks
-SortIncludes: CaseInsensitive
+SortIncludes: true
 SpacesInLineCommentPrefix:
     Maximum: 1
 TabWidth: 4

dirkolbrich commented Sep 15, 2023

View reviewed changes

aothms reviewed Sep 16, 2023

View reviewed changes

dirkolbrich force-pushed the v0.8.0-clang-format-test branch from 8505cdb to c478dbd Compare September 17, 2023 10:30

dirkolbrich changed the title ~~clang-format: first setting - DO NOT MERGE~~ clang-format: second format test - DO NOT MERGE Sep 17, 2023

dirkolbrich changed the title ~~clang-format: second format test - DO NOT MERGE~~ clang-format: add styles and reformat ifcparse files Sep 18, 2023

dirkolbrich force-pushed the v0.8.0-clang-format-test branch 2 times, most recently from 9383e48 to 57aabbd Compare September 29, 2023 09:50

dirkolbrich added 4 commits October 7, 2023 13:49

clang-format: add style file

fa0fb00

clang-format: no column length, no short blocks

5dfdd4e

clang-format: format test

6d96d71

clang-format: remove duplicates from base style

5affa86

clang-format: align license blocks

b494b7d

- split up some very long lines

dirkolbrich force-pushed the v0.8.0-clang-format-test branch from 57aabbd to b494b7d Compare October 7, 2023 11:52

aothms merged commit 616a969 into IfcOpenShell:v0.8.0 Oct 12, 2023
2 of 4 checks passed

dirkolbrich deleted the v0.8.0-clang-format-test branch October 23, 2023 13:40

clang-format: add styles and reformat ifcparse files #3761

clang-format: add styles and reformat ifcparse files #3761

Conversation

dirkolbrich commented Sep 15, 2023

dirkolbrich commented Sep 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aothms commented Sep 16, 2023

dirkolbrich commented Sep 16, 2023 via email

aothms commented Sep 16, 2023

dirkolbrich commented Sep 16, 2023

aothms commented Sep 16, 2023

dirkolbrich commented Sep 17, 2023

dirkolbrich commented Sep 17, 2023 • edited

dirkolbrich commented Sep 17, 2023

aothms commented Sep 17, 2023

dirkolbrich commented Sep 17, 2023

dirkolbrich commented Sep 18, 2023

dirkolbrich commented Sep 25, 2023

dirkolbrich commented Oct 2, 2023

dirkolbrich commented Oct 9, 2023

Krande commented Oct 12, 2023

dirkolbrich commented Oct 12, 2023

aothms commented Oct 12, 2023

aothms commented Oct 13, 2023

dirkolbrich commented Sep 15, 2023 •

edited

dirkolbrich commented Sep 17, 2023 •

edited