Proposed Changes/Updates to Console

[ezzieyguywuf]

PREFACE

As a result of a conversation that was started in wmayer's code-review thread for the App::Link merge, I have been working on a pull request that very specifically eliminates the need for the FC_CONSOLE_FMT macro.

Here, I have put together a proof-of-concept for an alternative to the FC_CONSOLE_FMT macro.

Now, as I posted here (on the same thread), the more I worked to incorporate this seemingly simple update, the more I found myself re-working other parts of the Console code base. In response to this post, @wmayer stated:

I don't think there is a need to throw over board all our current logging mechanism. There might be more mature solutions out there but for our purposes our implementation always was sufficient.

I wanted to take the time to address these concerns, as well as to discuss the general problems I'm seeing in the existing code base. Rather that continue to derail wmayer's original thread, I thought it best to start a new thread for this topic.

Introduction

I think the best way to start is actually by going over some of the changes I have in this branch of my personal FreeCAD git repository. You can follow along via the link I'll post or my cloning a local copy to your personal machine.

Please be aware that the code in this branch, in its current state, is broken. It does not compile. Rather, I am merely using it as a discussion point.

Console.h, and splitting up Class Definitions

There is a healthy discussion here on StackOverflow regarding translation units, and restricting oneself to one class definition per header/cpp file pair.

The idea is that, as much as possible, a developer should attempt to only define a single class in a given header file. In my personal experience, I have not found the need to deviate from this rule of thumb. In fact, whenever I have found myself tempted to define a second class in the same header, or even sometimes when I find myself forward-declaring* another class, it's turned out in every case that I was simply writing poor code to begin with. In every case (and again, I want to stress here that this is my own personal experience), I was able to do enough research, and learn a bit more about coding practices, in order to eliminate the need to do these things.

*(I realize there are times that this is necessary, but again, personally I've found this to be a rare case.)

If you take a look at Console.h in master, you'll notice that there are many macros defined, after which you find a forward-declaration of ConsoleSingleton . Later, you'll find a class definition for ConsoleObserver, then ConsoleSingleton itself (why not just move this definition in front of ConsoleObserver?!), then there's some classes that derive from ConsoleObserver.....

Well, I don't know about you, but I went into "Console.h" looking for a class definition for a class called "Console". Not only did I not find that, but I'm left wondering - what in the heck does "Console" do? Or rather, what is it intended to do?

I can guess from the documentation of ConsoleSingleton that the ConsoleSingleton class itself is intended to be the actual "Console", and then from examining its methods I can see that it is intended to give the user a single interface for emitting a Message, Log, Warning, etc... to any of the attached so-called "observers".

I found this very confusing, and difficult to understand.

If you look at the Console.h in my branch, you'll notice that aside from the (very nice!!) documentation, I've mostly taken everything out.

Where did it go? Into their own headers:

• ConsoleObserver.h defines the abstract base class ConsoleObserver
• ConsoleObserverFile.h and ConsoleObserverStd.h describe two distinct concrete implementations of the ConsoleObserver interface
• ConsoleEnums describe some enums that are used by the various ConsoleXXX classes to communicate certain information
• ConsoleEvent.h and ConsoleOutput.h describe some helper classes that are used by....
• ConsoleSingleton.h.

I think this change makes it a bit easier to understand what is going on. The various parts of the FreeCAD codebase that currently #include <Base/Console.h> will change to #include <Base/ConsoleSingleton.h>. This makes it extremely clear to future developers what is going on - the user has requested the use of a Singleton called Console. When examining the code, they'll realize that this was in order to acquire certain logging capabilities.

What's more, if someone wants/needs to understand what's going on in Console, they'll immediately know that ConsoleSingelton.h is the entry point, and can start there.

What is ConsoleObserver's Job?

It's easy (?) to see from the code that ConsoleObserver is intended to allow ConsoleSingleton to store a set of ConsoleObserver's in order to operate on them in the future. This does, indeed, appear to be some permutation of the Observer Design Pattern.

If ConsoleObserver is intended to be an interface, then, why is it not a pure abstract class? As it stands, I believe it could be instantiated, however this would result in a runtime error as Console.cpp does not define a default implementation for any of the virtual methods.

You'll notice that my ConsoleObserver is pure abstract - this makes it clear that it cannot and should not be instantiated on its own. Rather, it must be sub-classed.

In the current implementation of ConsoleObserver, there are a set of boolean values that are kept in public scope. It is unclear at first glance what these are for, but it is made somewhat clear in other places of code that they are used to establish which logging facilities are available in the various derived classes.

I changed this a bit in my implementation by specifically created a HasXXX method for each logging level. I'm actually not fully satisfied with this solution either, but I wanted to mention it because the current design in ConsoleObserver is very obscure, and difficult to understand.

Finally, it seems like ConsoleObserver wishes to have a text representation of its various derived classes for some reason. I changed this implementation a bit, by removing the burden from the derived classes and storing the name directly in ConsoleObserver. The only "overhead" this adds is that any derived class must explicitly call ConsoleObserver's constructor when it itself is being constructed.

What is the relationship between ConsoleObserver and ConsoleSingleton?

While I eventually figured this out (see above), it's not immediately clear. This is exacerbated by the fact that both have similar public method signatures. Both ConsoleObserver and ConsoleSingleton inclode Message, Log, Error, etc... methods.

ConsoleSingleton, on the other hand, also has NotifyXXX methods. It's not clear at first why this is.

After digging around a bit, I believe that the main reason for this split in ConsoleSingleton is the desire to allow for sprintf-style argument substition in the Message, Log, etc... methods, while still providing a simple, non-substitution method of printing messages.

I'm not entirely sure this is necessary. At the very least, I can say that using the template method, as I do in my branch does not require both, as can be seen in the proof-of-concept linked earlier.

Why do these two classes have matching function names? Are they intended to be interchangeable? Is it just a coincidence?

In general, it seems to me that the ConsoleObserver interface should not be user-facing. Or perhaps, better said, I don't think typical FreeCAD developers should need to worry about ConsoleObserver. Rather, only ConsoleObserver developers (i.e., someone wishing to create a new observer. Maybe logging on a network or something) should need to worry about this classe.

Most, if not all, developers (users) will be interacting with ConsoleObserver strictly through ConsoleSingleton. This seems to be the intent, at least.

If that's the case, I propose that ConsoleObserver does not need all these different methods. Rather, in order to simply things, something like:

Code: Select all

class ConsoleObserver{
    public:
        //snip
        void Print(const std::string& message, ConsoleMessageLevel level);
        //snip
}:

Where ConsoleMessageLevel is simple enum. I know that I mentioned a similar approach in wmayer's original thread, and that it was disputed, however if we accept the assumption that the user should primarily interact with ConsoleObserver through ConsoleSingleton, then I think this change makes sense.

What in the world is a console anyway?

So, I suppose this really brings us to the crux of the thing. After diving this deep into Console.h, I found myself with more questions than answers.

Ultimately, though, it appears that ConsoleSingleton was/is intended to provided a logging facility for the FreeCAD developer. This appears to be similar to the functionality provided by python's logging module. I've also come across spdlog, which is a c++ library that does almost the same thing as python's logging module.

The idea behind both of these libraries is that a developer should have an simple, single-interface to provide logging information. This interface should have the ability to attach any number of "sinks", such that the developer doesn't worry about where the information is going, but rather simply what the information is, and how important it is.

In this manner, any new developer can be told something along the lines of "use Logger.Debug a LOT, use Logger.Info often, use Logger.Warning sometimes, use Logger.Error when absolutely necessary" etc... These are simple guidelines to follow, and these new developers don't have to worry about what happens with this information.

Rather, someone else can decide that for Release builds, log messages below Warning are not displayed (by default) etc.. Also, some method of turning logging levels on/off can be provided for other developers (this is what Console.h's macros do today) so that debugging can be done more easily.

This understanding of what Console.h is trying to do, along with my personal experience using python's logging module, is what prompted my question regarding reinventing the wheel.

While I can understand the reluctance to add yet another dependency to FreeCAD, I wonder - do we need to? Why can't something like this by added as a git submodule and built alongside FreeCAD?

Heck, even if we don't want to use spdlog, why not use boost.log? I don't know anything about this library other than that it exists, but we already depend on boost for other things, so why not this?

Conclusion

This post is intended mostly to spark conversation and discussion. The original challenge that was posted by wmayer is:

There might be more mature solutions out there but for our purposes our implementation always was sufficient.

While the current solution might be sufficient, I would argue that it is (in my opinion) poorly written. I can agree that it gets the job done, but it is difficult to understand, and thus difficult to maintain.

Further, I generally take issue with the practice within the FreeCAD code base of defining multiple classes in a single header file. I believe that this practice makes the code base more daunting than it needs to be.

Similarly (though much less importantly - this is mostly a personal preference), I take issue with the "dumping" of all the header and source files into the same folder. You'll notice in my branch that i've created a src/Base/Console directory in which all of the ConsoleXXX.cpp files live. This does a few things:

1. de-clutters src/Base
2. Groups together source files that are related
3. Simplifies src/Base/CMakeLists.txt

Again, this is more of a personal preference, but I do believe that this can serve to improve the readability of our code base. Perhaps the approach that I took on my Part2 Workbench is a bit extreme (notice the depth of the include directory), but maybe it is also a step in the right direction?

(Again, this code is currently in development, and i"m only linking it to illustrate a ponit).

In this structure, Notice how clean the App sub-directory is. It is very clear that App/Part2.cpp is the entry point - if not solely from the filename, then by examining App/CMakeLists.txt.

It is also clear I can expect multiple different Features and Properties (currently, those are mostly empty...), and that I can find them in their respective sub-directories.

Anywho, I realize this post is now approaching a staggeringly long length, so I will end this here and hope for some lively conversation.

[fosselius]

Great write up!

The idea is that, as much as possible, a developer should attempt to only define a single class in a given header file. In my personal experience, I have not found the need to deviate from this rule of thumb. In fact, whenever I have found myself tempted to define a second class in the same header, or even sometimes when I find myself forward-declaring* another class, it's turned out in every case that I was simply writing poor code to begin with. In every case (and again, I want to stress here that this is my own personal experience), I was able to do enough research, and learn a bit more about coding practices, in order to eliminate the need to do these things.

I hate to have 10 files with 10 lines in, i rather have one 100 lines file, it improves the readability a lot! but as things grow it quickly becomes ugly and hard to maintain..

As for dependencies, I used to do my own daily builds, but as we moved to py3 and qt5 i have not put in the effort to be able to build again... i blame appimg and daily builds repo...

we should provide a docker image or VM image for developers

[wmayer]

The idea is that, as much as possible, a developer should attempt to only define a single class in a given header file. In my personal experience, I have not found the need to deviate from this rule of thumb. In fact, whenever I have found myself tempted to define a second class in the same header, or even sometimes when I find myself forward-declaring* another class, it's turned out in every case that I was simply writing poor code to begin with.

Forcing a rule where each class must be moved to its own header and source file doesn't make much sense IMO. What does it improve if you have classes that only consists of constructor and destructor to have them in their own files?
The C++ core guidelines don't recommend this either: https://isocpp.github.io/CppCoreGuideli ... s-of-files
And if even the designer of the language said this then it cannot be completely wrong

Furthermore, it slows-down the build process because for each translation unit the compiler again and again has to read in the same header files while when many small classes are inside the same file the related headers must be read in only once.

Projects like cotire do exactly this: one approach of it is to copy the code of many source files to a single file and build this instead. The other approach is to use pre-compiled headers (PCH) where you create one pch for one target and this includes header files of 3rd party libraries.

For FreeCAD we recently applied pch on Windows for all targets and we could reduce the build time by ~40%.

While I can understand the reluctance to add yet another dependency to FreeCAD, I wonder - do we need to? Why can't something like this by added as a git submodule and built alongside FreeCAD?

Heck, even if we don't want to use spdlog, why not use boost.log? I don't know anything about this library other than that it exists, but we already depend on boost for other things, so why not this?

FreeCAD already has a very long list of 3rd party libraries and it makes it very difficult for the maintainers on all the various platforms to keep this working. So, if we really need new functions then the very first thing must be to check the already used libraries.

To all the questions and remarks about the Console class and its observers there are for sure some aspects that can be improved.

[ezzieyguywuf]

Regarding dependencies, here is a minimal example which uses the spdlog library, but does not depend on it being installed. Rather, it pulls in the code using "git submodule".

Please note that I've only tested that this code compiles and runs - I believe there may be issues trying to package it, as the spdlog code may need to be incorporated into the "run.me" target using something like "add_executable(run.me main.cpp ${spdlog_SRCS})" or some such - I'm only trying to illustrate the point right now.

The code can be tested using:

Code: Select all

git clone https://gitlab.com/ezzieyguywuf_cpp_examples/GitSubmodule.git
cd GitSubmodule
mkdir build
cd build
cmake ..
make
./run.me

Forcing a rule where each class must be moved to its own header and source file doesn't make much sense IMO

I'm not necessarily suggesting that we force a rule blindly across-the-board. Rather, I'm suggesting that we attempt to clean up some of our code base by using this rule-of-thumb as appropriate.

What does it improve if you have classes that only consists of constructor and destructor to have them in their own files?

I will answer your question with another question: what good does it do to have a class which consists only of a constructor and destructor?

Furthermore, it slows-down the build process because for each translation unit the compiler again and again has to read in the same header files while when many small classes are inside the same file the related headers must be read in only once.

While I don't doubt the validity of this statement I wonder: how much slower will the compile really be if each class is in its own header?

Projects like cotire do exactly this: one approach of it is to copy the code of many source files to a single file and build this instead. The other approach is to use pre-compiled headers (PCH) where you create one pch for one target and this includes header files of 3rd party libraries.

I was not aware of cotire until you mentioned it. However, would it not be more beneficial to keep the code base in separate files and then use cotire after-the-fact, and get the "best of both worlds"?

I realize it may seem like I'm evangilizing "put all classes into their own files!" And maybe I am - this conversation has forced me to do some research and soul searching, and so I guess perhaps I may be changing my mind about what I believe is "best". As for now, though, my personal preference is still to split things up. This is the approach I've taken in my (much smaller than FreeCAD) projects such as OccWrapper, and I found that it made development for me much smoother and less troublesome.

I hate to have 10 files with 10 lines in, i rather have one 100 lines file, it improves the readability a lot! but as things grow it quickly becomes ugly and hard to maintain..

Again, I think I would have to disagree with this - I would prefer to have the 10 files with 10 lines each.

Alas, I fear this portion of the conversation has devolved into a bit of "preferences war", akin to vim vs emacs. Perhaps there is no "one right" answer, but rather it will always come to to a matter of personal preference.

The existence of the C++ Core Guidelines link is a bit damning to my preference

[wmayer]

I will answer your question with another question: what good does it do to have a class which consists only of a constructor and destructor?

Some view providers set a different icon than its base class to show in the tree view. This way it's easy to distinguish different solid types.
But we have also a couple of feature types that only implement the execute() method but don't have much code inside.

While I don't doubt the validity of this statement I wonder: how much slower will the compile really be if each class is in its own header?

Well, it's hard to say but I think it can easily become 20%. And for our travis builds this matters because we are always near to our time limits there if something in a base header changes.

[ezzieyguywuf]

Well, with regards to splitting class definitions into multiple files: I tried out the alternative where I took my existing codebase of OccWrapper (see include directory here) and converted it to more of a "monolith header file" style, which can be seen here.

Immediately, you'll notice that the include directory is cleaner due to less files.

While updating the srcs directory to incorporate these changes, I was able to appreciate the simplification of reducing the number of headers. While I haven't done anything other than update the tests, I think if I spent some more time coding using this paradigm, I would probably see what the overall benefits are.

Further, upon some reflection as well as conversation in ##c++ on freenode, I realized that my current preference is probably due to the fact that I learned object-oriented programming by reading the java documentation. While I read this probably 10 or so years ago (so the documentation may have changed), I believe at that time it was strongly suggested to split code up in this way.

It will take some work for me, but I can cede the point on this one and will start familiarizing myself with this different way of doing things.

[sgrogan]

Regarding dependencies, here is a minimal example which uses the spdlog library, but does not depend on it being installed. Rather, it pulls in the code using "git submodule".

Many build systems don't support git. On our own PPA the git info is removed. Hence the version.h problem.

[ezzieyguywuf]

Many build systems don't support git. On our own PPA the git info is removed. Hence the version.h problem.

What do you mean "the git info is removed"?

Git is not part of the "build system" - it is simply a tool used to manage revisions of the code base. One could say that it is a dependency of FreeCAD, since it is managed using git.

[kkremitzki]

What he means is that the .git directory is rm -rf'ed automatically in a cleanup step we don't control so that doing things like git rev-list --count HEAD or git submodule no longer work.

[Kunda1]

Related: issue #4014 https://forum.freecadweb.org/viewtopic.php?f=42&t=37216