Add doxygen comments to itemdef.h

[MinerDad7]

I've added doxygen comments to itemdef.h. I also removed a few function declarations from IWritableItemDefManager since they are already declared in its parent (IItemDefManager).

[ShadowNinja]

This is C++, we can use line comments. ;-)

[MinerDad7]

I was converting the existing comment into a doxygen group. Line comments (I believe you are talking about // comments) do not work for that. Doxygen will just ignore the comment. You'll get the group (because of the /* @{ */ line) but it won't have a title.

I can do one of the following:

1 - Remove the grouping tags (the @name, @{, and @} stuff). The resulting html document will still show all the members but it won't group them as "Basic item properties", etc
2 - Rewrite it as

/**
 * @name Basic item properties
 */

This feels a little too much for just the one line
3 - Rewrite it as

/**
 * @name Basic item properties
 * @{
 */

I prefer the look of /*@{*/ as a single line for the beginning of groups over this.

[ShadowNinja]

It seems like a lot of this isn't helpfull, it's just repeating what the code says. For example:

+   /**
+    * Serializes the tool
+    *
+    * @param os Output stream
+    * @param protocol_version The protocol version
+    */
    void serialize(std::ostream &os, u16 protocol_version) const;

[MinerDad7]

Yes, some of the functions are self-explanatory. You might have noticed that I didn't comment any of the constructors.

I was going for full coverage (except for constructors and deconstructors unless there is something important there [personal preference]). But if you'd rather not have the redundant comments, I'm fine with removing them.

[PilzAdam]

This are exactly the kind of comments that we don't want, see http://dev.minetest.net/Code_style_guidelines#Comments

[MinerDad7]

Which of the comments don't you want? Most of the comments were existing, I just converted them to doxygen.

Looking over it again, I think the only ones that are "uninformative" are the serialize and unserialize functions. Are there other ones that you find useless?

[MinerDad7]

I removed the serialize, unserialize, and ItemDefinition::type. I modified the IWritableItemDefManager::registerItem comment.

I amended my commit instead of adding a new commit. Is that ok?

[celeron55]

Comments should be written by people who have considerable insight into what the code does. Otherwise the comments end up just repeating what any programmer could immediately figure out from the code by themselves and thus they will not be useful at all.

If a comment is not useful, it is only in the way and will only confuse things.

I think MinerDad7 shouldn't be doing this to begin with.

[MinerDad7]

Per http://irc.minetest.ru/minetest-dev/2014-12-14#i_4064169, I've updated the pull request to use mainly class comments with information about some of the functions in the class comment.

I left in the doxygen groupings since celeron55 was ok with them but I've changed them to the /// syntax instead of the /** **/ syntax (per ShadowNinja's comment)

[celeron55]

You should use the wording "returns", "overwrites" and such in these. "will *" is just clumsy.

[celeron55]

Having all method explanations in the class description is a bit weird. But we can't have direct explanations for the methods without actually having it next to the methods, which means the class comment must be moved in practice to either itemdef.cpp or split to IItemDefManager and IWritableItemDefManager. This is kind of stupid.

Okay, I tried splitting it like that and moving the method descriptions to the methods. It doesn't really work and the descriptions are lost in the flood of auto-generated stuff. What you have here is the best doxygen can do in this case, as long as the comments pointing to CItemDefManager are added to IItemDefManager and IWritableItemDefManager.

In addition to those two things I don't think I have more complaints.

[celeron55]

Yeah so this works: https://gist.github.com/celeron55/2795fefc8b2ac3592efb

[MinerDad7]

I've updated the commit with your diff plus one more removal of "will" (will remove).

[celeron55]

Discussion about this still going on: http://irc.minetest.ru/minetest-dev/2014-12-16#i_4065832

[paramat]

No activity for 2 years.

[Zeno-]

+1 for close

[paramat]

I agree.