pharo-project / pharo-vm

This is the VM used by Pharo
http://pharo.org
Other
113 stars 68 forks source link

fix warnings related to multiple include of the same header file #840

Closed RenaudFondeur closed 1 month ago

RenaudFondeur commented 1 month ago

small change to fix the 'warning, attempt to include <sys/stat.h> / for e.g. mkdir / a second time' happening in Slang during C code generation.

the class AbstractImageAccess include sys/stat.h and the subclasses don't override declareCVarsIn: so we get the warning for each of them each time a CCodeGenerator is created.

guillep commented 1 month ago

Hmm I don't like this change. Yes, it fixes the issue, but it looks more like a dirty patch than a solution (fixing the symptom, not the cause).

What is the cause? Maybe the include be set in some other class?

PalumboN commented 1 month ago

Helloooo, I come with comments (but not solutions :P).

Mmm I see two problems (one in the solution and other in the issue):

  1. I agree with Guille. Anyway, does this solution work? As both subclasses are overriding the method, the include won't be set in the header file. Am I right? Was it tested?

  2. Soooo, the real problem is that declareCVarsIn: is called for each concrete class. There are two concrete subclasses for AbstractComposedImageAccess class, then this method will be executed twice (logging the waring inside addHeaderFile: method):

    declareCVarsIn: aCCodeGenerator
    
    aCCodeGenerator
        addHeaderFile:'<sys/stat.h> /* for e.g. mkdir */'

I don't see a good solution for this: moved to another hierarchy as Guille says seems to be the best, but in that case the header definition will be disconnected from the code that uses it...

It looks like Slang is not prepared for the subclassification that we try to do here (having 2 subclasses), at least for this kind of definitions.

guillep commented 1 month ago

We talked with @RenaudFondeur and we decided to add a couple of good comments to these methods, at least they will not remain just empty. It looks like a good compromise solution.