Am 10.09.2013 um 09:01 schrieb Vincent van Ravesteijn <v...@lyx.org>:
> Op 6-9-2013 11:59, Jean-Marc Lasgouttes schreef: >> 28/08/2013 22:45, Vincent van Ravesteijn: >>> Hi all, >>> >>> I think it's time to start thinking about what is needed before we can >>> release beta2. >>> >>> Please reply if you know about any issues so that I won't be able to >>> overlook them. >> >> I would like to merge the branch features/kill-tex2lyx-define. It solves >> cleanly the problem with automake subdirs and is a long awaited cleanup IMO. >> The branch consists of small self-contained commits and is easy to review >> IMO. >> > > I had a look at the branch, and I have the following comments: > > Snip comments answered by JMarc already... > - 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. 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? Stephan
