- The commit about splitting the encoding stuff and moving helper functions
into a new class, should really have some explanation why this is needed, and
why we would need a new class for this. The construction around CharInfo
becomes rather strange. It is defined in Encoding.cpp, which indicates that it
should not be used outside Encoding.cpp. It is now used outside this file in
BufferEncoding.cpp. Because of this, there are a lot of static functions added
to the Encoding class just to allow BufferEncoding to use CharInfo as an
incomplete type. Moreover, most of these functions aren't even used. Why is
this moved into a new class ? What was the problem that needed to be solved ?
Can't we solve it with a private header file ?
- The file src/BufferEncoding.cpp starts with a comment that it is Encoding.cpp
- The class name is BufferEncodings so why don't we call the file
BufferEncodings.cpp ? (I know Encodings in Encoding.cpp.., but is that a good
reason?)
- The following includes in BufferEncoding.cpp are not needed:
#include "BufferList.h"
#include "Lexer.h"
#include "LyXRC.h"
#include "support/debug.h"
#include "support/gettext.h"
#include "support/FileName.h"
#include "support/textutils.h"
#include "support/unicode.h"
#include <boost/cstdint.hpp>
#include <sstream>
The root problem is the fact Encoding(s) classes are used both by LyX and
tex2lyx
and these classes are using the Buffer stuff.
This forces to link tex2lyx with the Buffer etc. making it another LyX binary
or split the classes somehow. Originally it was done by a dirty hack -
using the same code even without making a copy and compiling it twice with
different
compiler flags. That's not nice. Neither with C nor with C++.
Nevertheless I'm standing on the "shoulder of giants" and want to change the
code base
only slightly if possible. I cannot use modern tools like Eclipse for
refactoring the
code.
Well the "giants" have made a mess in certain areas of the code. And I
believe this is increasing the mess a bit.
Regarding the details:
* BufferEncoding.cpp is a manual copy of Encoding.cpp
- that's why the wrong comment
- that's why the name of the file
- that's why the superfluous includes
* CharInfo was a private struct
- I've made a class to make it more functional but didn't want to make it public
- It should be used inside Encoding(s) only
- Yes, one can move it to a separate public class with header file
- Yes, that's why the static methods in Encoding.cpp …
Who is we?
Well, I have no other choice than to fix this myself in the coming days
and to prepare the release of beta 2 in the weekend, or to reject this
branch for now and prepare the release of beta 2.
Vincent
