Menu
▾
▴
RE: [Opencxx-users] C++ design patterns
|
From: Grzegorz J. <ja...@he...> - 2004-03-12 03:57:32
|
On Thu, 11 Mar 2004, SF Markus Elfring wrote:
> > > Most interfaces appear to me as if they are C functions.
> > > My goal is a check if more data structures can be encapsulated in a real C++ style.
> > > Was a class API refactoring considered?
> > Yes.
>
> Hello,
>
> I looked at the source files. Now I suggest some changes to the code
> in detail. I dare to present several refactorings to see if you can
> possibly agree with them. The next generation class library would be
> developed. The current (old) implementation would call the new one to
> achieve backward compatibility. It may happen that others prefer to
> use the new interface from the beginning.
That makes sense.
>
> 1. types.h:
> Why is not the direct and smaller header <stddef.h> used?
types.h is hell --- too much conditional compilation, lumps different
things together (e.g. dynamic loading and gc). I recently broke it into
several separate input files. Please review the code in
'sandbox_jakacki_frontend1' branch and let me know if this issue is still
there.
> 2. all files:
> 2.1 Should the type name "uint" be replaced by "size_t"?
Perhaps yes. There is another reason why I would like to see uint killed---a
typedef like this one IMO does not bring sufficient value (see [1]).
> 2.2 Several virtual destructors are missing. They are needed to derive useful classes.
> Does that mean that several structures are "concrete classes" that would be specified as "final" in Java?
That may be the case. Some classes are designed to be derived from, some
are "final". In particular classes that have value semantics (= define
operator() and copy ctor and generally behaves as values) IMO almost
always should be "final". But there may also be omissions. Please bring
the names of these classes and I will comment on each of them
particularily.
> 2.3 Inline code documentation would be necessary to better understand
> the relationships between the data structures. How do you think about
> tools like "DocBook" or "Doxygen"?
I think "YES!". That's a perfect point. IMHO it is much easier to keep docs
in sync, also it is clearly visible what is documented, what's not. Some
time ago (1.5 years?...) we were talking about using Synopsis or Doxygen for
this purpose. There is an outstanding "TODO" item about moving the existing
docs into sources and implementing Makefiles to extract them. I am still
looking for somebody to do it. Once infrastructure is in place I pledge to
fill in the gaps in developers docs.
> 2.4 Can the methods "Translate..." be moved into "Translators"?
I am not sure if I understand. Currently "Translate..." methods are in
"Walkers", together with "Typeof..." methods. If you mean separating
"Translate..." from "Typeof..." then I think this is a good idea. Please
also see list archives on SF for the discussion about extracting an
"AbstractWalker" base class from existing wakers (let me know if you
cannot find it, I should be able to dig it up from my mailboxes).
Apart of that there is some additional cruft in Walker public iface
which does not belong there (e.g. static TranslateArgDeclList2()).
> 2.5 Copyright notice: express -> expressed
I am not a lawyer and English is not my first language, but I think you
are wrong here (see [2]).
> 2.6 Where are the copy constructors and assignment operators? Are
> compiler generated implementations used for them?
Again, this very much depends on the class semantics. Sometimes compiler
generated implementations are OK. Please note, that due to GC even when
class has value semantics and it contains (garbage collected) pointers,
default copy ctor and assignment operator are ok. On the other hand,
I suspect that in several places those functions should be blocked
(by privation), but are not.
Please point out the places where you think something is wrong and I will
comment on them. It may be that in some places this is messed up.
> 2.7 Please consider constant methods like it is in the waiting queue
> for the class "Member".
Present code does not employ 'const' in many places where it should,
perhaps because const did not have reasonable support in compilers when
Chiba was designing OpenC++. I would love to see const-correctness
improved, but:
* it is easy to break backward compatibility (on the other hand
it can be worked around by using e.g. const_<T>::type and
giving client an option to define const_<T>::type == T)
* it is not easy to retrofit const into existing code. I tried to
do this several times on different products and almost always
you end up with a snow-ball effect: one const here requires
two consts there, and they require further eight const there
and you have to paddle a lot before you reach the closure.
If you see any places that 'const' may be injected in relatively painless
way, let's inject it.
> 2.8 Was thread safety and concurrent access considered?
I am not aware of any such efforts. Before going into these areas I would
however first try to determine if people really need it, because it looks
like a huge work. I would expect that not many compilation tools use threads
in parsing or DOM manipulation, but I might be wrong (the examples that
spring to my mind are syntax-higlighting editors or some huge and
sophisticates IDEs that run many independent agents to modify ASTs; but this
requires much more caution than just thread safety).
> 3. ptree.h:
> 3.1 I would prefer class templates instead of the macros
> "ResearvedWordDecl", "PtreeStatementDecl" and "PtreeExprDecl".
I was thinking along the same lines, however I decided not to go for
templates for very pragmatic reason: ptree.h has to be compilable by occ
itself (ptree.h is included by mop.h), so I was afraid that unresolved issues
with templates parsing will surface here.
I was not satisfied with defines either, so I wrote simple script that
generates all these declarations at configuration time. Please review it
(it is in sandbox_jakacki_frontend1 branch) and let me know what you
think about this solution.
> 3.2 There are several classes with the same methods "What" and
> "Translate". Why were they not moved into another base class like
> "Translatable" or "Typeof" that would be used for multiple inheritance
> together with the class "NonLeaf"?
I am under impression that each 'Ptree...::What()' returns something else
(discriminator), similarily each 'Ptree...::Translate()' calls different
method from Walker iface. Can you point out what classes have the same
What() methods?
>
> 4. walker.h, mop.h, parse.h:
> 4.1 The classes "Parser", "Walker" and "Class" contain "fat"
> interfaces. The sections "public", "protected" and "private" should be
> ordered.
Is this one item or two? As far as I understand "fat interface" means that
it handles lots of different functionalities. I don't see any connections
of "obesity" to ordering of public, protected and private parts of iface,
please correct me if I am wrong.
However, if you mean two distinct items, then:
(a) "fat interface". To some extent I agree. As for "Walker": I think that
only "Translate..." functions fit there, and "Typeof..." functions
should be moved out, to something like another "Walker" (TypingWalker?).
Also functions like "NewScope()" hardly fit there at first glance.
"Class": it lumps together at least three responsibilities: representation
of C++ class (Members(), Name()), being a MOP metaclass (Translate...()),
and serving as access point for different functionalities of OpenC++
(e.g. RegisterNewModifier(), ErrorMessage()). It may be argued that the
idea of OpenC++ is that first two should be lumped together, but
I do not agree, as this disables the independent reuse of parser and
static analyzer without MOP.
"Parser": I don't think this interface is very "fat". The public
interface is in fact too lean as for my taste, because being an outsider
you cannot use Parser just to parse, say, an expression. There are some
functions that IMO do not fit in protected iface (e.g. SyntaxError()),
but I think these are minor problems.
(b) ordering of public, protected, private: I do not see much value in
ordering those sections apart from aestethics (there are some technical
merits to it, but only if you depend on data layout in memory; we
don't). There is some logic to the existing ordering, I don't think we
should reorder. Also different people have different views on
aesthetisc, so it is difficult to defend the point that one layout is
better than another. I am aware of at least two major styles (Lakos and
Stroustrup/Sutter) and variations (e.g. if you say "private:" again
for separating private data from private functions), for me both are OK
if used consequently. Please let me know if I miss your point.
> 4.2 The replacement of pointers with C++ references would lead to a complete redesign.
That's for sure and I have one advice here. I have been programming for 14
years, using C++ for 10 years and for more than 5 years I do C++ every day.
That's to make the subsequent statement heavier :-)
Complete redesign is very costly (in open source project that means it takes
*lots* of time) and after you build the thing anew you initially get lower
quality than the original and you need to put another lot of work in it to
make it equally good. From this point it begins to be nice, since you have
better structure. However reaching this point is far more difficult than it
originally seems. Ask Chiba how many manmonths has he put into OpenC++,
estimate contributions from different people (see README, AUTHORS and 'cvs
history' output), divide by number of hours you can spend on it every week
and I bet you arrive at years.
The way that I would strongly recommend is to redesign components one by one
and go from the old design to the new design in evolutionary steps. The
smaller the components you replace, the easier it is to manage. Sometimes it
pays off to refactor the old code to insulate some part of it before
rewritting them. That way you have the running thing all the time. Also if
you get bored or occupied with something else on the way, your work up to
date is not lost.
OpenC++ is not perfect, but is fairly modular and unlike many other projects
it seems fairly easy to improve it in small steps. Please let me know what
you think about this strategy.
> 4.3 Limits like "MaxOptions" can be avoided if STL collections would be used.
True (applies also to other limits).
> 5. hash.h:
> How do see the future of the class "HashTable" in comparison to "std::hash_map"?
First, hash_map is not standard yet, so I would be reluctant to use it.
Second, IMO map would be enough for a while, at least until somebody
complains about efficiency (and that should be trivial to upgrade to
hash_map).
Third, YES, it would be fantastic if somebody simplifies HashTable with STL.
That would get one bug off my head (I happen to hit premature garbage
collection somewhere in HashTable).
>
> 6. ptree-core.h:
> 6.1 Please correct...
> inline std::ostream& operator << (std::ostream& s, Ptree* p)
> {
> p->Write(s);
> return s;
> }
> to
> inline std::ostream& operator << (std::ostream& s, const Ptree& p)
> {
> p.Write(s);
> return s;
> }
Why do you think it is better?
> 6.2 Can the class "PtreeIter" benefit from the STL iterator rules?
I don't think so. PtreeIter just was designed with another protocol in mind
and it should stay this way. However, that may be useful to have another
PtreeArray iterator that models STL iterator concept. Do you think it would
be possible to implement it as PtreeIter wrapper?
>
> 7. token.h:
> Why are the functions from the file <ctype.h> not reused?
Perhaps they were not available when the code was created. I would not touch
this are, unless we need to do some other work in token.h . It works as it
is, it does not show any bugs, rewritting this part would have only
educational purpose and this is not the aim of this project. Do you think it
is reasonable?
>
> I am curious how you will react on these ideas. I hope that I do not bother and annoy you.
Not this time :-)
> Is this a useful contribution?
Sure.
I would also very much appreciate if you could review
'sandbox_jakacki_frontend1' branch and let me know your comments. Perhaps
you could also have a look at file TODO, as it somewhat shows a direction in
which I would like to head.
Also I encourage you to get yourself your sandbox, since many of your
proposals are worth implementing immediately. If you decide to contribute,
please send me your SourceForge ID, so that I can add you to the project.
Best regards
Grzegorz
Footnotes:
[1] "A typedef declaration creates an alias for an existing type, not a
new type. A typedef, therefore, gives only ilusion of type safety."
J. Lakos "Large Scale C++ Software Design", p. 30
[2] I just found it with Google: "The law divides warranties into 'express'
and 'implied' warranties. Express warranties are any promises to back
up the product that the seller expresses either in writing or orally."
http://cobrands.public.findlaw.com/newcontent/consumerlaw/chp8_a.html
##################################################################
# Grzegorz Jakacki Huada Electronic Design #
# Senior Engineer, CAD Dept. 1 Gaojiayuan, Chaoyang #
# tel. +86-10-64365577 x2074 Beijing 100015, China #
# Copyright (C) 2004 Grzegorz Jakacki, HED. All Rights Reserved. #
##################################################################
|
