Issue 6326553071845376: Doxygen configuration file (Closed)
DescriptionDoxygen configuration file
To generate documentation, run 'doxygen' (no command line arguments required) in
the root directory of the project.
This configuration assumes installation of Graphviz for generation of call
graphs. Graphviz executable must be in the PATH. Precompiled binaries available
for Windows.
http://www.graphviz.org/
Patch Set 1 #MessagesTotal messages: 12
I personally don't think we need this. I don't see either myself nor any code newbies looking at the documentation produced by Doxygen and have any more understanding then just by looking at the source code. If you insist we can certainly add this though.
On 2014/08/05 13:20:10, Oleksandr wrote: > I personally don't think we need this. I don't see either myself nor any code > newbies looking at the documentation produced by Doxygen and have any more > understanding then just by looking at the source code. If you insist we can > certainly add this though. I will repost my comment regarding it (src: http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/...) I see that it's doxygen comment, but it's not a reason we need it. Firstly the comments should explain non trivial things and reasons of the application, some peculiarities of the implementation, restrictions. Pure textural repeating of the code as well as additional lines like "/* <empty>" makes only the noise. Arguments that it's needed for the generated documentation don't convince, because the proper navigation in the IDE makes the generated documentation useless, we will not use it, but will have to support it.
On 2014/08/05 13:20:10, Oleksandr wrote: > II don't see either myself nor any code > newbies looking at the documentation produced by Doxygen As a code newbie to this code base, one of the very first things I did was to set up doxygen for it. Even without a single doxygen-specific comment in the code, I got a lot out of the call graphs and the class summaries.
On 2014/08/05 13:26:43, sergei wrote: > I will repost my comment regarding it > Firstly the comments should explain non trivial things and reasons of the > application, some peculiarities of the implementation, restrictions. That's quite a personal view of what comments are for. It has become clear that you wish everyone shared your personal views, but that's not the case. Some comments are for explaining the code. Some comments are for documenting the code. Reading documentation for code is much, much faster than learning the same things from reading the code. The reason to document code is not principally to explain code that someone needs to read, but rather to provide them with enough summary information that they might avoid reading code they don't need to read. Well-documented code is far easier to maintain, because you don't spend nearly as much time chasing dead ends. Even for code you do need to read, it's faster to read documented code, since it provide information about context to make sense of details that otherwise has to be reconstructed. Your WIP review http://codereview.adblockplus.org/5126032940072960/ took me far longer to read than I want exactly because it was almost wholly undocumented. I you believe in quality code, you should also believe in documented code.
Sorry, I didn't provide the arguments, because some of them are exactly what Oleksandr said On 2014/08/05 13:20:10, Oleksandr wrote: > I personally don't think we need this. I don't see either myself nor any code > newbies looking at the documentation produced by Doxygen and have any more > understanding then just by looking at the source code. If you insist we can > certainly add this though. We don't host this documentation somewhere, so the developer has to perform manual action to build it firstly locally. I think that the most probably the new developer will be fed up enough by trying to compile the project and won't have a desire to fiddle around to build the documentation. Also I don't find comments which repeat the code useful, examples: /** 87 * Copy constructor is deleted 88 */ 89 BSTR_Param(const BSTR_Param&); // = delete 90 91 /** 92 * Move constructor is deleted 93 */ 94 BSTR_Param(BSTR_Param&&); // = delete /** 119 * Constructor from std::wstring. 120 */ 121 BSTR_ParamArgument(const std::wstring& s); /** 152 * Conversion operator to std::wstring 153 */ 154 operator std::wstring() const; I don't mind about the summary information, but the examples above are far from it. One can find an example, how I see it, in v8.h https://code.google.com/p/v8/source/browse/branches/bleeding_edge/include/v8.h . Apparently there will not be a huge evolution of the project, for now it's good without the documentation, why do we need to add it? It definitely takes the effort to add it and to support it. It's much better to think how we can be more descriptive using the less code and at the same time be warned about the wrong usage by the compiler.
On 2014/08/06 13:19:12, sergei wrote: > I think that the most probably the > new developer will be fed up enough by trying to compile the project and won't > have a desire to fiddle around to build the documentation. You've obviously never used doxygen, and you're speaking out of ignorance. All you do is to type 'doxygen' on the command line, and it just works. (You have to get the configuration file set up first, but that's what I've provided). Doxygen is one of the very few build tools that I've never, ever, even once had any particular trouble with. > Also I don't find comments which repeat the code useful, examples: > /** > 87 * Copy constructor is deleted > 88 */ > 89 BSTR_Param(const BSTR_Param&); // = delete More ignorance. There would be no need for these comments if the current version of doxygen properly handled C++11 "= delete". The comments are a substitute for that. > It definitely takes the > effort to add it and to support it. If you don't want to document your own code, then don't.
ok, I don't see a lot of reasons why this shouldn't be committed if we already have it working. Looks like we can pretty much not spend any time for it's support. If we have any troubles with it in future we can just remove it. So this LGTM. On 2014/08/06 14:54:22, Eric wrote: > On 2014/08/06 13:19:12, sergei wrote: > > I think that the most probably the > > new developer will be fed up enough by trying to compile the project and won't > > have a desire to fiddle around to build the documentation. > > You've obviously never used doxygen, and you're speaking out of ignorance. > > All you do is to type 'doxygen' on the command line, and it just works. (You > have to get the configuration file set up first, but that's what I've provided). > Doxygen is one of the very few build tools that I've never, ever, even once had > any particular trouble with. > > > Also I don't find comments which repeat the code useful, examples: > > /** > > 87 * Copy constructor is deleted > > 88 */ > > 89 BSTR_Param(const BSTR_Param&); // = delete > > More ignorance. There would be no need for these comments if the current version > of doxygen properly handled C++11 "= delete". The comments are a substitute for > that. > > > It definitely takes the > > effort to add it and to support it. > > If you don't want to document your own code, then don't.
We ended up discussing two issues here: 1. Should we add a Doxyfile to the repository so that people can generate docs from it? 2. To what extent should we use Doxygen comments? This review is just about the first issue. I don't think it hurts to maintain a Doxyfile, if that's all there is to it. The generated documentation is useful even without Doxygen comments, and it's just an isolated file. On the second issue I've got a more complex opinion. I agree with Sergei that the kind of comments he pointed out above are completely pointless. But there are two kinds of comments we should consider separately: A. Comments in the code, or documenting private API B. Comments documenting public API (e.g. the public members of a type) When it comes to A, my opinion is rather ruthless: I think they're most often a fallback for when you cannot manage to express your intent in code, not a shortcut around it. I always nag about those in reviews and try to suggest better naming/structure instead. Type B, on the other hand, can be quite useful IMO. They communicate intent, as opposed to implementation details. I'd be worried that they get out of sync if we didn't have mandatory reviews, but we do. The core code has pretty good jsdoc coverage, and I think it helps more than it hurts: https://adblockplus.org/jsdoc/adblockplus/ However, all the comments Sergei pointed out above are of type B, and still not useful IMO. Before we add any kind of Doxygen comments, we need to agree on how we want to use them in this project. There is no "my code" and "your code", there is only "our code", and it all needs to meet the same standards. On 2014/08/06 16:02:24, Oleksandr wrote: > ok, I don't see a lot of reasons why this shouldn't be committed if we already > have it working. Looks like we can pretty much not spend any time for it's > support. If we have any troubles with it in future we can just remove it. So > this LGTM. > > On 2014/08/06 14:54:22, Eric wrote: > > On 2014/08/06 13:19:12, sergei wrote: > > > I think that the most probably the > > > new developer will be fed up enough by trying to compile the project and > won't > > > have a desire to fiddle around to build the documentation. > > > > You've obviously never used doxygen, and you're speaking out of ignorance. > > > > All you do is to type 'doxygen' on the command line, and it just works. (You > > have to get the configuration file set up first, but that's what I've > provided). > > Doxygen is one of the very few build tools that I've never, ever, even once > had > > any particular trouble with. > > > > > Also I don't find comments which repeat the code useful, examples: > > > /** > > > 87 * Copy constructor is deleted > > > 88 */ > > > 89 BSTR_Param(const BSTR_Param&); // = delete > > > > More ignorance. There would be no need for these comments if the current > version > > of doxygen properly handled C++11 "= delete". The comments are a substitute > for > > that. > > > > > It definitely takes the > > > effort to add it and to support it. > > > > If you don't want to document your own code, then don't.
On 2014/08/07 08:14:10, Felix H. Dahlke wrote: > When it comes to A, my opinion is rather ruthless: I think they're most often a > fallback for when you cannot manage to express your intent in code, not a > shortcut around it. I mostly agree with this, with one important exception. There's no syntax in most programming languages to express the invariants of private data except by comment. (I only know of one such that's of production maturity: Eiffel. Even SPARK doesn't do type invariants yet.) So for non-trivial invariants, I'll comment them. It's not ideal, since Doxygen doesn't have a clean way of separating public interface documentation from private internals documentation, but it's better than not having it at all.
I'm committing this and closing the review. The issue on whether to have the capability to use documentation annotations seems settled. This review is not the best venue to debate larger policy issues on how use them.
On 2014/08/07 14:37:56, Eric wrote: > I'm committing this and closing the review. > > The issue on whether to have the capability to use documentation annotations > seems settled. This review is not the best venue to debate larger policy issues > on how use them. Alright, LGTM. But as I said, let's not add any Doxygen comments before we've figured out if/how to use them. 
|
