depthai-core icon indicating copy to clipboard operation
depthai-core copied to clipboard

multiple conflicting/hiding variables and class members -> compiler warnings/fails

Open diablodale opened this issue 2 years ago • 3 comments

In many places of the codebase (including the headers of the precompiled Windows depthai distribution), the code uses duplicate variable names which conflict and/or hide other variables (e.g. class members). Leading to compiler warnings and in some cases compiler failures.

Fix is to not conflict/hide variables with same names. Can be found easily with a compile. And fixed easily by changing conflicting names (e.g. following Google style guide).

Setup

  • all compilers and OS
  • throughout codebase, yet can also be seen in the headers of precompiled Windows distrib: depthai-core-v2.11.1-win64

Repro

  1. Enable warnings and errors in compiler. e.g. in MSVC it is with /W4
  2. Compile an app that uses the depthai-core-v2.11.1-win64 and simple code that cascades to headers like common/Rect.hpp or pipeline/Node.hpp. Most any code, even just a dai device, will do this.

Result

...
[build] Microsoft (R) C/C++ Optimizing Compiler Version 19.29.30136 for x64
[build] Copyright (C) Microsoft Corporation.  All rights reserved.
[build] 
[build] C:\repos-nobackup\depthai-core-v2.11.1-win64\include\depthai-shared/common/Rect.hpp(100): error C2220: the following warning is treated as an error
[build] C:\repos-nobackup\depthai-core-v2.11.1-win64\include\depthai-shared/common/Rect.hpp(100): warning C4458: declaration of 'width' hides class member
[build] C:\repos-nobackup\depthai-core-v2.11.1-win64\include\depthai-shared/common/Rect.hpp(129): note: see declaration of 'dai::Rect::width'
[build] C:\repos-nobackup\depthai-core-v2.11.1-win64\include\depthai-shared/common/Rect.hpp(100): warning C4458: declaration of 'height' hides class member
[build] C:\repos-nobackup\depthai-core-v2.11.1-win64\include\depthai-shared/common/Rect.hpp(130): note: see declaration of 'dai::Rect::height'
[build] C:\repos-nobackup\depthai-core-v2.11.1-win64\include\depthai-shared/common/Rect.hpp(116): warning C4458: declaration of 'width' hides class member
[build] C:\repos-nobackup\depthai-core-v2.11.1-win64\include\depthai-shared/common/Rect.hpp(129): note: see declaration of 'dai::Rect::width'
[build] C:\repos-nobackup\depthai-core-v2.11.1-win64\include\depthai-shared/common/Rect.hpp(116): warning C4458: declaration of 'height' hides class member
[build] C:\repos-nobackup\depthai-core-v2.11.1-win64\include\depthai-shared/common/Rect.hpp(130): note: see declaration of 'dai::Rect::height'
[build] C:\repos-nobackup\depthai-core-v2.11.1-win64\include\depthai-shared/datatype/RawFeatureTrackerConfig.hpp(99): warning C4305: 'initializing': truncation from 'double' to 'float'
[build] C:\repos-nobackup\depthai-core-v2.11.1-win64\include\depthai-shared/datatype/RawFeatureTrackerConfig.hpp(105): warning C4305: 'initializing': truncation from 'double' to 'float'
[build] C:\repos-nobackup\depthai-core-v2.11.1-win64\include\depthai-shared/datatype/RawFeatureTrackerConfig.hpp(174): warning C4305: 'initializing': truncation from 'double' to 'float'
[build] C:\repos-nobackup\depthai-core-v2.11.1-win64\include\depthai\pipeline\node/SPIOut.hpp(62): warning C4458: declaration of 'id' hides class member
[build] C:\repos-nobackup\depthai-core-v2.11.1-win64\include\depthai\pipeline\Node.hpp(199): note: see declaration of 'dai::Node::id'
...

Expected

No warnings or errors. To not use conflicting variable names.

Workaround

No good workaround. Setting flags to ignore errors/warnings would need to be somehow only applied to these headers. But the headers are not isolated to depthai compile units and instead included in user compile units. Therefore disabling the compiler errors/warnings would then put the user code at risk by not reporting problems in that user's code.

diablodale avatar Nov 09 '21 18:11 diablodale

For example, you can change https://github.com/luxonis/depthai-core/blob/41de567ea52443368224145b9e54a4f858abaeb8/include/depthai/pipeline/node/SPIOut.hpp#L46-L48 to

    void setBusId(int newId) {
        properties.busId = newId;
    }

This was necessary for my code to work. I also did the same kind of thing in https://github.com/luxonis/depthai-shared/blob/main/include/depthai-shared/common/Rect.hpp

DBraun avatar Aug 23 '22 19:08 DBraun

@DBraun, luxonis is often happy to receive PRs. Since there are many of these, a reasonable approach could be for someone like you to submit a PR that fixes a subset of them. And then over time, the issue is progressively resolved. Could you submit a PR with some of your changes and then we can all look/discuss. :-)

diablodale avatar Aug 24 '22 03:08 diablodale

@diablodale Sorry, when I said "you" I meant Luxonis ;). There's probably a lot of places to refactor, and I didn't catch them all, just the ones that caused errors for me. I would prefer Luxonis make the changes since they know the codebase better.

DBraun avatar Aug 24 '22 03:08 DBraun