Skip to content

Feature/c3d various - #290

Open
martinfalk wants to merge 4 commits into
masterfrom
feature/c3d-various
Open

martinfalk wants to merge 4 commits into
masterfrom
feature/c3d-various

Conversation

@martinfalk

Copy link
Copy Markdown
Contributor

Changes:

  • added MetaData to C3D
  • extended C3D to create a new instance from a C3D frame subset
  • added processors for cropping frames in C3D data

@github-actions

Copy link
Copy Markdown

Cpp-Linter Report ⚠️

Some files did not pass the configured checks!

clang-tidy (v22.1.2) reports: 7 concern(s)
  • misc/c3d/include/inviwo/c3d/datastructures/c3ddata.h:47:1: warning: [misc-multiple-inheritance]

    inheriting multiple classes that aren't pure virtual is discouraged

       47 | class IVW_MODULE_C3D_API C3D : public ezc3d::c3d, public MetaDataOwner {
          | ^
  • misc/c3d/include/inviwo/c3d/datastructures/c3ddata.h:47:26: warning: [cppcoreguidelines-special-member-functions]

    class 'C3D' defines a copy constructor, a copy assignment operator, a move constructor and a move assignment operator but does not define a destructor

       47 | class IVW_MODULE_C3D_API C3D : public ezc3d::c3d, public MetaDataOwner {
          |                          ^
  • misc/c3d/src/processors/c3dcrop.cpp:98:67: warning: [bugprone-narrowing-conversions]

    narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'std::int64_t' (aka 'long') is implementation-defined

       98 |         dataframefilters::intMatch(0, filters::NumberComp::Equal, index)};
          |                                                                   ^
  • misc/c3d/src/processors/c3dsequencecrop.cpp:82:51: warning: [performance-unnecessary-value-param]

    the parameter 'c3d' of type 'std::shared_ptr' is copied for each invocation but only used as a const reference; consider making it a const reference

       82 | DataSequence<C3D> crop(std::shared_ptr<const C3D> c3d, const DataFrameLookUp& lookup,
          |                                                   ^
          |                        const                     &
  • misc/c3d/src/processors/c3dsequencecrop.cpp:87:67: warning: [bugprone-narrowing-conversions]

    narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'std::int64_t' (aka 'long') is implementation-defined

       87 |         dataframefilters::intMatch(0, filters::NumberComp::Equal, index)};
          |                                                                   ^
  • misc/c3d/src/processors/c3dsequencecrop.cpp:141:21: warning: [misc-const-correctness]

    variable 'done' of type 'util::OnScopeExit' can be declared 'const'

      141 |                     util::OnScopeExit done{[&]() { progress(1.0); }};
          |                     ^
          |                                       const 
  • misc/c3d/src/processors/c3dtodataframe.cpp:99:23: warning: [clang-diagnostic-pessimizing-move]

    moving a temporary object prevents copy elision

       99 |                       std::move(std::views::repeat(static_cast<int>(index), nbFrames) |
          |                       ^
    /home/runner/work/modules/modules/inviwo/misc/c3d/src/processors/c3dtodataframe.cpp:99:23: note: remove std::move call here
       99 |                       std::move(std::views::repeat(static_cast<int>(index), nbFrames) |
          |                       ^~~~~~~~~~
      100 |                                 std::ranges::to<std::vector<int>>()))
          |                                                                    ~

Have any feedback or feature suggestions? Share it here.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cpp-linter Review

Used clang-tidy v22.1.2

Only 6 out of 7 clang-tidy concerns fit within this pull request's diff.

Click here for the full clang-tidy patch
diff --git a/misc/c3d/src/processors/c3dsequencecrop.cpp b/misc/c3d/src/processors/c3dsequencecrop.cpp
index 373eb2c..12f496a 100644
--- a/misc/c3d/src/processors/c3dsequencecrop.cpp
+++ b/misc/c3d/src/processors/c3dsequencecrop.cpp
@@ -82 +82 @@ struct DataFrameLookUp {
-DataSequence<C3D> crop(std::shared_ptr<const C3D> c3d, const DataFrameLookUp& lookup,
+DataSequence<C3D> crop(const std::shared_ptr<const C3D>& c3d, const DataFrameLookUp& lookup,
@@ -141 +141 @@ void C3DSequenceCrop::process() {
-                    util::OnScopeExit done{[&]() { progress(1.0); }};
+                    const util::OnScopeExit done{[&]() { progress(1.0); }};

Have any feedback or feature suggestions? Share it here.

namespace inviwo {

class IVW_MODULE_C3D_API C3D : public ezc3d::c3d {
class IVW_MODULE_C3D_API C3D : public ezc3d::c3d, public MetaDataOwner {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy diagnostic

misc/c3d/include/inviwo/c3d/datastructures/c3ddata.h:47:1: warning: [misc-multiple-inheritance]

inheriting multiple classes that aren't pure virtual is discouraged

   47 | class IVW_MODULE_C3D_API C3D : public ezc3d::c3d, public MetaDataOwner {
      | ^

clang-tidy diagnostic

misc/c3d/include/inviwo/c3d/datastructures/c3ddata.h:47:26: warning: [cppcoreguidelines-special-member-functions]

class 'C3D' defines a copy constructor, a copy assignment operator, a move constructor and a move assignment operator but does not define a destructor

   47 | class IVW_MODULE_C3D_API C3D : public ezc3d::c3d, public MetaDataOwner {
      |                          ^

const auto& dataFrame = *dataframe_.getData();

const std::vector<dataframefilters::ItemFilter> filters{
dataframefilters::intMatch(0, filters::NumberComp::Equal, index)};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy diagnostic

misc/c3d/src/processors/c3dcrop.cpp:98:67: warning: [bugprone-narrowing-conversions]

narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'std::int64_t' (aka 'long') is implementation-defined

   98 |         dataframefilters::intMatch(0, filters::NumberComp::Equal, index)};
      |                                                                   ^

std::shared_ptr<const Column> lastFrameColumn;
};

DataSequence<C3D> crop(std::shared_ptr<const C3D> c3d, const DataFrameLookUp& lookup,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy diagnostics

  • the parameter 'c3d' of type 'std::shared_ptr' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
Suggested change
DataSequence<C3D> crop(std::shared_ptr<const C3D> c3d, const DataFrameLookUp& lookup,
DataSequence<C3D> crop(const std::shared_ptr<const C3D>& c3d, const DataFrameLookUp& lookup,

std::stop_token stop, pool::Progress progress) -> DataSequence<C3D> {
if (stop.stop_requested()) return {};
progress(0.0);
util::OnScopeExit done{[&]() { progress(1.0); }};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy diagnostics

Suggested change
util::OnScopeExit done{[&]() { progress(1.0); }};
const util::OnScopeExit done{[&]() { progress(1.0); }};

index = c3d->getMetaData<SizeMetaData>("sequenceIndex", index);

const std::vector<dataframefilters::ItemFilter> filters{
dataframefilters::intMatch(0, filters::NumberComp::Equal, index)};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy diagnostic

misc/c3d/src/processors/c3dsequencecrop.cpp:87:67: warning: [bugprone-narrowing-conversions]

narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'std::int64_t' (aka 'long') is implementation-defined

   87 |         dataframefilters::intMatch(0, filters::NumberComp::Equal, index)};
      |                                                                   ^

df->addColumn("Frame", std::move(frameIndices));
df->addColumn("Time", std::move(timeValues));
df->addColumn("sequenceIndex",
std::move(std::views::repeat(static_cast<int>(index), nbFrames) |

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy diagnostic

misc/c3d/src/processors/c3dtodataframe.cpp:99:23: warning: [clang-diagnostic-pessimizing-move]

moving a temporary object prevents copy elision

   99 |                       std::move(std::views::repeat(static_cast<int>(index), nbFrames) |
      |                       ^
/home/runner/work/modules/modules/inviwo/misc/c3d/src/processors/c3dtodataframe.cpp:99:23: note: remove std::move call here
   99 |                       std::move(std::views::repeat(static_cast<int>(index), nbFrames) |
      |                       ^~~~~~~~~~
  100 |                                 std::ranges::to<std::vector<int>>()))
      |                                                                    ~

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants