Am 25.09.2013 um 20:59 schrieb Vincent van Ravesteijn <v...@lyx.org>:
> >>> - 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. I did it myself. BTW, I think there was a bug in the parser… The removed lines - if (suffixIs(info.mathcommand, '}')) - info.flags |= CharInfoMathNoTermination; were above the assignment of the info.mathcommand member variable. Stephan
