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:
- In the commit logs, it is nowhere mentioned why the cleanups are
necessary.
- In commit ed55131e8, the "automake subdirs" is silently introduced. As
this change has caused such a lot of work, it should really be a
separate commit, and should explain that the other commits were needed
to allow this. In this same commit, the TEX2LYX defines are removed.
This should be a separate commit, or be merged with the work that was
needed in removing the TEX2LYX defines.
- 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 change to src/tex2lyx/tex2lyx.cpp is unexpected in a commit that
says it removes something from ModuleList.cpp.
Vincent
