pitchfork icon indicating copy to clipboard operation
pitchfork copied to clipboard

Feedback on cxx-pflR1 (2018-11-14)

Open FlorianWolters opened this issue 6 years ago • 3 comments

Hello,

I'm pretty late to the party and stumbled across Pitchfork two days ago. I'm very interested in the future of the C++ ecosystem (build/tool integration as well as modernization/modern usage of the C++ language itself). I do think a common filesystem for C++ (and C) source code would be great. So, thanks for you effort!

I've been using a (self-created) CMake-based Convention over Configuration (CoC) for some time now and I think I'm pretty good and confident using CMake and a component-centric approach using a small-to-medium scale code base. I.e. I know what works in general and what not. My framework is almost zero-conf, except dependency management: dependencies are listed in CMakeLists.txt, but find_package calls are propagated transitively to downstream projects by the system. Using a package manager is not possible yet.

Suprisingly (or not) I'm already using the same concepts (library-centric, one library per project, distinction between "app" and "lib" projects, etc.) and file system layout as defined in the PFL (with some minor modifications).

I'm using this issue to provide some feedback to you. Please tell me if I can help with anything else (though my time is pretty limited).

  • Question: Where do we store Interface Definition Language (IDL), e.g. Protobuf or CORBA? They are code, imo. Therefore data does not fit. Since now I've been using a proto/idl directory. But that does not scale (other IDLs) and is against not-introducing additional directories.

  • I recommend using singular instead of plural for all directory names. A lot of projects mix singular and plural directory names, e.g. LLVM. That should be avoided in the PFL, imo. Example: doc (instead of docs), test (instead of tests), etc. Reasons:

    1. Consistency: src and include are singular too.
    2. Compactness+Expressiveness: It safes one character per directory but the readability is the same.
  • What is the reason for using the .hpp file extension for header files? Though I agree that this makes things more explicit for C++, there are reasons against it. Reasons:

    1. The C++ Core Guidelines recommend using .h.
    2. Using .h for both C++ and C simplifies build system adoption.
  • I don't think that the build directory should be specified by a C++ filesystem layout convention.

    1. Against "best-practices": out-of-source by its strict definition means "outside of a project directory <project_name>", not <project_name>/_build.
    2. Inconsistency: If a build directory is mentioned, why is not install/stage directory mentioned?
    3. Scope: The PFL should focus on a C++ project filesystem source layout (keep the scope as narrow as possible). Therefore I suggest dropping all mentions of a build directory. If it is not dropped, it should be mentioned last at least (its the first item in section 1.3).
  • I do not have a strong opinion about external, data, tools, libs and extras yet. Do we really need that much directories? Also I do not understand why "submodules" are necessary (it seems it is a pretty rare use case for big code bases only - but I can always use multiple projects instead). Maybe I have to read the sections again.

  • I think using An Elaborated Option 2 is a very good and scalable approach (did I mention that I'm also using app for "production" executable files?). I like the idea to be able to add executable-specific source files in that directory. By doing that the library of the project is not "polluted" with project-local files only.

  • 1.2. Project Files: I suggest removing the sentence "Other files should not appear in the root of the project.". Though the sentence does not state shall (mandatory requirement), it is an optional requirement that would be violated by the existance of other pretty common files in the root directory. Examples:

    1. CHANGELOG.md: Keep a CHANGELOG
    2. CONTRIBUTING.md: Contributing Guidelines
  • 2.4. tests/: In my opinion the layout of that directory has to be specified by the PFL. Otherwise we have to consider that .cpp test source files are stored in multiple arbitrary subdirectories below that directory. Usually each .cpp test source file is compiled (with a main() entry point function) to a test executable. We have to consider that the same project provides header and source files for testing only (e.g. an Object Mother class). In my opinion they do not belong into the src/include directories, but below the test directory. Without a convention for the layout of that directory, it is unneccessary hard for the build system to determine which .cpp files have to be linked against the test framework (e.g. GTest) and which .cpp files are part of the testing library part of the project. I suggest specifying the directories (app, include and src) below test as follows (the rest is listed for illustration only):

    └───test
        ├───app
        │   │   simple_integration_test.cpp
        │   │
        │   └───complex_integration_test
        │       │   complex_integration_test.cpp
        │       │
        │       ├───include
        │       └───src
        ├───include
        │   └───fw
        │       └───testing
        │               my_test_util_class.h
        │               testing.h
        │
        └───src
            └───fw
                └───testing
                        my_test_util_class.cpp
    
  • 3.2.1. Where is the .test convention derived from? I don't like using dots in filenames (except at the beginning or for the extension) and I also think that is is very unusual. Using <name>_test.cpp would be better, since it could map to the class name used inside of that file (e.g. class StringTest in file string_test.h). Reasons:

    1. Standard-Conformance: There are coding standards out there (e.g. AUTOSAR) that do require such a 1:1-mapping between names.
    2. No "ambiguous" file extenstion (.test.cpp or .cpp?)

Again: I think the PFL is an awesome idea. The C++ ecosystem lacks so much compared to other (mostly managed) languages.

A common file system layout and a common/spread CoC build system (potentially using CMake) enforcing that layout would be super nice, imo.

If that CoC build system would (as opt-ins) support most of the open-source software C++ development tools (e.g. Cppcheck, Doxygen, clang-tidy), beginners could have a fun-time starting with Modern C++.

As it is now, I cannot recommend a (sane) beginner to start programming using C++, since dealing with the ecosystem is a pain in the a**.

Resources

Though, I did not completely read all of the following resources, I think they should be taken into account and/or referenced.

In addition, they may provide information for further improvements. I may add additional items in the future here.

My current filesystem suggestion

Note that the following listing does not include "resource" files yet, since it is pretty late here already.

But it includes everything related to source code files (except example).

  │   .clang-format
  │   .clang-tidy
  │   .cppcheck
  │   .gitignore
  │   CHANGELOG.md
  │   CONTRIBUTING.md
  │   LICENSE
  │   README.md
  │
  ├───app
  │   │   hello_world.cpp
  │   │   tower_of_hanoi.cpp
  │   │
  │   └───cli_greeter
  │       │   cli_greeter.cpp
  │       │
  │       ├───include
  │       │       greeter.h
  │       │
  │       └───src
  │               greeter.cpp
  │
  ├───include
  │   └───fw
  │           fw.h
  │           my_class.h
  │
  ├───src
  │   └───fw
  │           fw.cpp
  │           my_class.cpp
  │           my_class_test.cpp
  │
  └───test
      ├───app
      │   │   simple_integration_test.cpp
      │   │
      │   └───complex_integration_test
      │       │   complex_integration_test.cpp
      │       │
      │       ├───include
      │       └───src
      ├───include
      │   └───fw
      │       └───testing
      │               my_test_util_class.h
      │               testing.h
      │
      └───src
          └───fw
              └───testing
                      my_test_util_class.cpp

Regards

FlorianWolters avatar Nov 14 '18 23:11 FlorianWolters

Thanks for the lengthy write-up! Sorry to take a few days to get back to you, though.

Where do we store Interface Definition Language

You aren't the first person to ask. I'm open to suggestions. So far I've just said "Eh... put it in src/" but I think a more thorough answer is warranted.

I recommend using singular instead of plural for all directory names

I keep flip-flopping on the pluralization of directory names. I'll need to consolidate on one and commit to it.

What is the reason for using the .hpp file extension for header files?

I don't mandate a specific extension, but I do recommend one. I chose .hpp to specifically call out the distinction between .h files. There could be huge debate about this aspect. Also, this project was partially made in response to the Core Guidelines regarding aspects like file layout. The "Just be consistent" methodology keeps people in their own "islands of consistency".

I don't think that the build directory should be specified by a C++ filesystem layout convention.

I don't either. build/ and _build/ are reserved, but not required. The concept of an "install" directory is not mentioned because installing inside of the project directory is not common, and should be done inside of the build directory if not to some external path.

As for the distinction between executable sources and library sources, there is still a heated debate on the topic.

I suggest removing the sentence "Other files should not appear in the root of the project."

I'll add support for the common CHANGELOG and CONTRIBUTING files, but I'd still rather encourage most user-readable content to go in the docs/ directory.

tests/: In my opinion the layout of that directory has to be specified by the PFL. Otherwise we have to consider that .cpp test source files are stored in multiple arbitrary subdirectories below that directory.

If there's one thing less consistent that building a project, that would be testing a project. I'm not sure what the best test layout would look like. For some projects, individual .cpp files are generated straight to executables, and a whole large structure is not helpful. For those with large test infrastructure, more provisions are warranted. This varies wildly between build systems and testing libraries, harnesses, and methodologies. This may be the same basis for the problem of finding a good layout for distinguishing between executable sources and library sources.

Where is the .test convention derived from?

John Lakos originally recommends using something.t.cpp. I'm not a fan of the needless brevity, so I adapted it to something.test.cpp. This particular point was actually hugely contentious when I posted it on my blog, and the "requirement" has been lowered to "recommendation."

A common file system layout and a common/spread CoC build system (potentially using CMake) enforcing that layout would be super nice, imo.

You may be interested in Evoke which is a convention-oriented build system. I've also written a pf_auto() function that automatically sets up a CMake-based build using the directory structure. You can see it used in the CMakeLists.txt for this project.

Thanks for all the feedback!

vector-of-bool avatar Nov 16 '18 23:11 vector-of-bool

One thing I'd like to say about tests: It imho doesn't make sense to have the test files directly next to the source files, because they have completely different prupose, requirements and are used in different ways from the "regular" source files. Just as a few examples of what I mean:

  • Unittests almost always have additional dependencies (e.g. the unittest framework). How do I specify which additional libraries I need?
  • For many simple projects, the build process is just lumping all cpp files together and compile it into a library/executable (maybe with the exception of architecture specific implementation files, that I usually put into an filder). If you put test source files into the same folder, you now need a separate mechanism (like naming convention) to differentiate which files to pick for which target.
  • I've encountered many cases, where I didn't have a one-to one mapping between the implementation file and the unittest files anyway (e.g. because I preferred to split up the unittests for a single cpp file into multiple test files due to it's size)
  • For better or for worse, regular library files and test files are not always build together (header only-libraries might not be build at all and e.g. if I build a 3rd party dependency as part of my project, I'm not always going to build and run the tests of that library),

So, I aknowledge, that there is a strong argument for putting unit-test files next to the files they test due to the strong coupling between them. However, my understanding is that one of the main purposes of this proposal is to make building, consumption, deploying and generally tooling of c++ projects easier and with respect to all those points, a unit-test file has to be handled differently and has different requirements than "normal" cpp files and as such I think they should be placed into a different directory.

Mike-Devel avatar Nov 18 '18 17:11 Mike-Devel

Thanks for the lengthy write-up! Sorry to take a few days to get back to you, though.

Where do we store Interface Definition Language

You aren't the first person to ask. I'm open to suggestions. So far I've just said "Eh... put it in src/" but I think a more thorough answer is warranted.

Seems some updates are needed in the "Open Questions" section of the document.

But the specific answer here might be simple: put them either in include/ or src/.

A native C++ project can certainly have source code files not conforming to the C++ standard. Consider C and assembly sources used to be included by the preprocessor (which is not necessarily a part of the same implementation of C++ being used to build the C++ source here). IDL does not differ in this sense.

Some loosely relavant: putting .bs into data/ seems wrong to me. This is also a form of human-readable source code, even it never build to program executable or library blobs. Or put it in docs/?

I recommend using singular instead of plural for all directory names

I keep flip-flopping on the pluralization of directory names. I'll need to consolidate on one and commit to it.

Or just provide the normative allowence of synonyms, with some predefined lists of alternative.

This is needed for some extra consistency. Consider include + src: would include + source or inc + src be strictly more correct? But anyway, include + src is already widely used for historical reasons, even in some standards. I don't think sticking to one choice is good enough. (There might be the amibguity of "source" constrast to "binary package" in the sense of the package management, though. Such use can be also used in a layout convention like this.)

It also allows to resolve t vs test by users themselves.

FrankHB avatar Jun 23 '21 06:06 FrankHB