Re: [openMSX-devel] ICC remarks

[Wouter V. <wou...@pi...>]

On Thu, 18 Dec 2003, Maarten ter Huurne wrote:

> Hi,
>
> I've been compiling openMSX with ICC (Intel's compiler) the last couple of
> days. Wouter and I solved all errors and warnings, what is left is the
> remarks (which like warnings, but less severe).
>
> There are two ways to remove remarks: change the code or disable specific
> remarks (they all have a unique ID). I would like some discussion about the
> different remarks. For some, because I don't fully understand their impact,
> for others because we would have to introduce new coding conventions to
> remove them.
>
> Below, I listed all the unique remarks that occur on the openMSX code, but not
> every instance of them (it's a very long list).
>
> remark #111: statement is unreachable
> These should be fixed, in my opinion. There are two instances: one in
> XRenderer (unfinished code) and one in ViewControl (I don't know this class).

See tcm's mail.


> remark #177: handler parameter "e" was declared but never referenced
> This happens if you write "catch (Exception &e)" and then don't use the "e"
> variable in the catch clause. Seems harmless to me (coming from a Java
> background).

Seems harmless to me too.


> remark #185: dynamic initialization in unreachable code
> This happens only on the following piece of code:
>
> void SDLRenderer<Pixel, zoom>::drawEffects()
> {
> 	// All of the current postprocessing steps require hi-res.
> 	if (LINE_ZOOM != 2) return;
>
> 	int scanlineAlpha = (settings.getScanlineAlpha()->getValue() * 255) / 100;
>
> Since LINE_ZOOM is a constant which depends on the "zoom" template parameter,
> the initialisation of scanlineAlpha can be unreachable indeed. But since any
> code that uses it (the rest of the method) is unreachable as well, I don't
> see anything problematic here. If anyone disagrees, please explain.

These remarks can be ignored.


> remark #271: trailing comma is nonstandard
> This happens if you put a comma after the last label in an enum declaration.
> What does "nonstandard" mean here: "violating the C++ spec" or
> "unconventional"? In the former case, we should fix it ofcourse, in the
> latter case I would like to keep the commas.

Somebody told me it's part of the standard. But I don't really care if
we change it or not.


> remark #279: controlling expression is constant
> This happens if you have a "while" with a guard that is always true or always
> false. Most remarks are about the PRT_DEBUG macro. Maybe we can change
> PRT_DEBUG into something without a fake "while" loop, to see whether the
> remaining remarks are useful or not.
> I use "while(true) { ... break; ... }" when it's not possible to evaluate the
> exit condition at the start of the loop. One alternative is to partially
> unroll the loop, but the resulting code duplication is worse in my opinion
> than the unclear control path of "break". Another alternative is to use "for
> (;;)" instead of "while(true)", that would probably avoid this remark, but it
> still uses the "break".
> To summarize, I'm not sure this one is about coding style or about possible
> bugs. I'd like some comments.

It's safer to have the loop in macros (I'll explain why if someone
asks). But it's probably even better to replace the macro with a
(static) function. However the disadvantge is that on a lot of places we
then have to change simple stuff like this:
   PRT_DEBUG("Variable i = " << i);
into:
   ostringstream os;
   os << "Variable i = " << i;
   Print::debug(os.str());
or make something special for int's like this:
   Print::debug("Variable i = " + stringOp::toString(i));

I prefer "while (true)" over "for(;;)". I agree we should try to avoid
loops like this, but duplicating part of the loop is certainly worse.

So, I vote to also ignore this remark.


> remark #304: access control not specified ("private" by default)
> This happens for example in:
>   class Joystick : public JoystickDevice, EventListener
> where EventListener is inherited as "private". I think it's clearer to
> explicitly put "private" in front of it.

Agreed.


> remark #383: value copied to temporary, reference to temporary used
> There seems to be one instance of this remark every time a (C-style) string
> contant is used to call a method which expects an (STL) const string&
> parameter. It would be problematic if the method keeps the reference, but
> afaik our methods copy the value if they need it for longer than the call
> itself. However, it is not simple to know 100% sure that no method uses the
> reference in the wrong way. Is there a way we can make the code safer without
> sacrificing too much performance? For example, what is the performance impact
> of passing string objects as values instead of references?

Performance impact is not that big, most STL implementation use
reference counted strings, so string copies are relativly cheap. However
I prefer to keep the references, it's very common practice to pass
strings this way.
Like you said methods that need to keep the string should copy it. I think
i've never seen code that violates this rule (it's not even so simple to
violate it, you already need something like a string& member variable).


> remark #444: destructor for base class "openmsx::MSXException" is not virtual
> This can be a serious problem in some cases, see this explanation and test
> case: http://lightconsulting.com/~thalakan/virtualtest1.cpp
> The remarks on openMSX code I saw so far, were about subclasses of
> MSXException, which has an empty destructor that is not virtual. Is this
> destructor also responsible for cleaning up the "message" field, or does that
> happen anyway?
> In case it's not strictly necessary, would we want to make all destructors of
> base classes virtual? What is the overhead of doing so? I guess 1 vtable
> entry and some additional calls on destruction, but performance critical
> parts of openMSX don't do dynamic allocation while emulation is running
> anyway.

We should fix these. Like you said a virtual destructor is not performance
critical in our case.
The problem is the following:

  class A { ... };              // non-virtual destructor
  class B : public A { .. };    // non-empty destructor
  vector<A*> v;
  v.push_back(new A());
  v.push_back(new B());
  for(vector<A*>::const_iterator it = v.begin(); it != v.end(); ++it) {
    delete *it;
  }

In the loop we destroy A object's. Because the A-destructor is
non-virtual, this destructor is used for both elements. The (non-empty)
B-destructor is thus never executed.

We've already had bugs like this in openMSX.


> remark #530: inline function [name] cannot be explicitly instantiated
> Happens on several video classes. May be important. Will investigate later.


> remark #810: conversion from [larger type] to [smaller type] may lose
> significant bits
> This is a remark about implicit narrowing conversions, for example converting
> a 64-bit int to a float, or a 32-bit int to a char. There are potentially
> dangerous, but can be correct. I'm used to programming Java, where all
> narrowing conversions must be made explicit (implicit conversion is a compile
> error). It's not hard to get used to, in my opinion. So we could consider to
> adopt the coding convention to use static_cast for all narrowing number
> conversions. Is this feasible?

I agree with static_cast<>. See also my (future) reply on tcm's mail.


> remark #869: parameter [name] was never referenced
> Similar to #177, but now for method parameters. Not a problem of itself, but
> it could be useful to spot candidates for refactoring. However, many
> inherited methods have this remark, when one implementation does use the
> parameter, but others do not. If my memory is correct, this remark can be
> avoided by not repeating the parameter name in the method definition, only
> the type, for example "ClassA::methodX(int)" instead of "ClassA::methodX(int
> n)". Do we want to adopt that as a coding convention?

You're right, not repeating the variable name makes this remark go away.
But I don't like it because then you lose a large part of the function's
documentation.

  MyClass::myMethod(int, int) {
    // TODO still need to implement this
  }

Should i really remember (ok, or look it up) what these two parameters
mean?


> remark #981: operands are evaluated in unspecified order
> I'm not sure exactly what triggers it, but it seems any expression with two
> function calls in it triggers it. If one of those function calls has a side
> effect, it could cause trouble.
> I prefer to avoid using functions having side effects altogether, instead of
> worrying about the order they are evaluated in. So in my opinion, we should
> disable this remark. But maybe someone disagrees, or I have interpreted it
> wrong?

I agree, we should disable it. Methods that have a side effect should, as
much as possible, return void.


> remark #1418: external definition with no prior declaration
> This seems to happen if something defined without being declared first. It
> seems some things in .cc files don't have a matching entry in the .hh. I'll
> have to investigate more before I can say something sensible about it.


> remark #1469: "cc" clobber ignored
> I have no idea what this means. It occurs on the "htons" and "htonl" calls in
> JoyNet.cc.


> While making this list, I fixed a few remarks that in my opinion obviously
> needed fixing. The remaing ones (that you just read) I'm not 100% sure about,
> so reactions are welcome.
>
> Bye,
> 		Maarten


Wouter