-
Notifications
You must be signed in to change notification settings - Fork 1
Index Repairing of RNTuple ROOT file #13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Index Repairing of RNTuple ROOT file #13
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
There were too many comments to post at once. Showing the first 10 out of 24. Check the log or trigger a new build to see more.
| @@ -1,6 +1,6 @@ | |||
| #include <benchmark/benchmark.h> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: 'benchmark/benchmark.h' file not found [clang-diagnostic-error]
#include <benchmark/benchmark.h>
^| "chr21:1-48129895", | ||
| "chrM:1-16571", | ||
| "chrY:2600000-2700000", | ||
| const std::vector<std::string> RegionQueryFixture::regions_ = {"1:1000000-1001000", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: invalid case style for variable 'regions_' [readability-identifier-naming]
| const std::vector<std::string> RegionQueryFixture::regions_ = {"1:1000000-1001000", | |
| const std::vector<std::string> RegionQueryFixture::regions = {"1:1000000-1001000", |
| BENCHMARK_REGISTER_F(RegionQueryFixture, TTree)->Args({0})->Args({3})->Args({6})->Args({9})->Unit(benchmark::kSecond); | ||
|
|
||
| BENCHMARK_REGISTER_F(RegionQueryFixture, RNTuple)->Args({0})->Args({3})->Args({6})->Args({9})->Unit(benchmark::kSecond); | ||
| BENCHMARK_REGISTER_F(RegionQueryFixture, TTree)->DenseRange(0, 17, 1)->Unit(benchmark::kSecond); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: function 'BENCHMARK_REGISTER_F' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
| BENCHMARK_REGISTER_F(RegionQueryFixture, TTree)->DenseRange(0, 17, 1)->Unit(benchmark::kSecond); | |
| BENCHMARK_REGISTER_Fstatic (RegionQueryFixture, TTree)->DenseRange(0, 17, 1)->Unit(benchmark::kSecond); |
|
|
||
| BENCHMARK_REGISTER_F(RegionQueryFixture, RNTuple)->Args({0})->Args({3})->Args({6})->Args({9})->Unit(benchmark::kSecond); | ||
| BENCHMARK_REGISTER_F(RegionQueryFixture, TTree)->DenseRange(0, 17, 1)->Unit(benchmark::kSecond); | ||
| BENCHMARK_REGISTER_F(RegionQueryFixture, RNTuple)->DenseRange(0, 17, 1)->Unit(benchmark::kSecond); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: function 'BENCHMARK_REGISTER_F' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
| BENCHMARK_REGISTER_F(RegionQueryFixture, RNTuple)->DenseRange(0, 17, 1)->Unit(benchmark::kSecond); | |
| BENCHMARK_REGISTER_Fstatic (RegionQueryFixture, RNTuple)->DenseRange(0, 17, 1)->Unit(benchmark::kSecond); |
| #ifndef RAMCORE_SAMTONTUPLE_H | ||
| #define RAMCORE_SAMTONTUPLE_H | ||
|
|
||
| #include <cstdint> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: 'cstdint' file not found [clang-diagnostic-error]
#include <cstdint>
^| void samtoramntuple_split_by_chromosome(const char *datafile, const char *output_prefix, int compression_algorithm, | ||
| uint32_t quality_policy, int num_threads = 4); | ||
| uint32_t quality_policy, int num_threads = 4, | ||
| const std::vector<std::string> &only_chromosomes = {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: parameter 6 is const-qualified in the function declaration; const-qualification of parameters only has an effect in function definitions [readability-avoid-const-params-in-decls]
| const std::vector<std::string> &only_chromosomes = {}); | |
| std::vector<std::string> &only_chromosomes = {}); |
| @@ -1,6 +1,7 @@ | |||
| #include "ramcore/RAMNTupleView.h" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: 'ramcore/RAMNTupleView.h' file not found [clang-diagnostic-error]
#include "ramcore/RAMNTupleView.h"
^| #include <iostream> | ||
| #include <cstring> | ||
| #include <limits> | ||
| #include <ROOT/RNTuple.hxx> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: included header limits is not used directly [misc-include-cleaner]
| #include <ROOT/RNTuple.hxx> | |
| #include <ROOT/RNTuple.hxx> |
| int rangeDelimiterPos = region.find("-", chrDelimiterPos); | ||
| if (rangeDelimiterPos == std::string::npos) { | ||
| try { | ||
| range_start = std::stoi(region.substr(chrDelimiterPos + 1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: no header providing "std::stoi" is directly included [misc-include-cleaner]
range_start = std::stoi(region.substr(chrDelimiterPos + 1));
^|
|
||
| auto recordView = reader->GetView<RAMNTupleRecord>("record"); | ||
| auto flagView = reader->GetView<uint16_t>("record.flag"); | ||
| auto refidView = reader->GetView<int32_t>("record.refid"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: no header providing "int32_t" is directly included [misc-include-cleaner]
src/ramcore/RAMNTupleView.cxx:1:
- #include <iostream>
+ #include <cstdint>
+ #include <iostream>There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
There were too many comments to post at once. Showing the first 10 out of 17. Check the log or trigger a new build to see more.
| @@ -1,6 +1,9 @@ | |||
| #include "ramcore/RAMNTupleView.h" | |||
| #include <cstdint> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: #includes are not sorted properly [llvm-include-order]
#include <cstdint>
^this fix will not be applied because it overlaps with another fix
src/ramcore/RAMNTupleView.cxx
Outdated
|
|
||
| stopwatch.Print(); | ||
|
|
||
| printf("Found %lld records in region %s\n", static_cast<long long>(count), query ? query : ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: do not call c-style vararg functions [cppcoreguidelines-pro-type-vararg]
printf("Found %lld records in region %s\n", static_cast<long long>(count), query ? query : "");
^
src/ramcore/SamToNTuple.cxx
Outdated
| #include <mutex> | ||
| #include <algorithm> | ||
| #include <unordered_set> | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: included header unordered_set is not used directly [misc-include-cleaner]
src/ramcore/SamToNTuple.cxx
Outdated
|
|
||
| auto rootFile = std::unique_ptr<TFile>(TFile::Open(treefile, "RECREATE")); | ||
| if (!rootFile || !rootFile->IsOpen()) { | ||
| printf("Failed to create RAM file %s\n", treefile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: do not call c-style vararg functions [cppcoreguidelines-pro-type-vararg]
printf("Failed to create RAM file %s\n", treefile);
^
src/ramcore/SamToNTuple.cxx
Outdated
| ramcore::SamParser parser; | ||
|
|
||
|
|
||
| int64_t mapped_count = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: no header providing "int64_t" is directly included [misc-include-cleaner]
int64_t mapped_count = 0;
^
src/ramcore/SamToNTuple.cxx
Outdated
|
|
||
|
|
||
| int64_t mapped_count = 0; | ||
| int32_t last_refid = -2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: no header providing "int32_t" is directly included [misc-include-cleaner]
int32_t last_refid = -2;
^| #include <filesystem> | ||
| #include <Rtypes.h> | ||
| #include <regex> | ||
| #include "../tools/ramview.cxx" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: included header regex is not used directly [misc-include-cleaner]
| #include "../tools/ramview.cxx" | |
| #include "../tools/ramview.cxx" |
| #include <filesystem> | ||
| #include <Rtypes.h> | ||
| #include <regex> | ||
| #include "../tools/ramview.cxx" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: suspicious #include of file with '.cxx' extension [bugprone-suspicious-include]
#include "../tools/ramview.cxx"
^
test/ramcoretests.cxx
Outdated
|
|
||
| testing::internal::GetCapturedStdout(); | ||
| testing::internal::CaptureStdout(); | ||
| ramview(ttreeFile, region, true, false, nullptr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: argument comment missing for literal argument 'cache' [bugprone-argument-comment]
| ramview(ttreeFile, region, true, false, nullptr); | |
| ramview(ttreeFile, region, /*cache=*/true, false, nullptr); |
test/ramcoretests.cxx
Outdated
|
|
||
| testing::internal::GetCapturedStdout(); | ||
| testing::internal::CaptureStdout(); | ||
| ramview(ttreeFile, region, true, false, nullptr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: argument comment missing for literal argument 'perfstats' [bugprone-argument-comment]
| ramview(ttreeFile, region, true, false, nullptr); | |
| ramview(ttreeFile, region, true, /*perfstats=*/false, nullptr); |
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (42.53%) is below the target coverage (85.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #13 +/- ##
==========================================
Coverage ? 53.02%
==========================================
Files ? 16
Lines ? 1439
Branches ? 739
==========================================
Hits ? 763
Misses ? 552
Partials ? 124
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
| #include <cstring> | ||
| #include <iostream> | ||
| #include <limits> | ||
| #include <string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: included header limits is not used directly [misc-include-cleaner]
| #include <string> | |
| #include <string> |
src/ramcore/SamToNTuple.cxx
Outdated
| #include <TFile.h> | ||
| #include <TROOT.h> | ||
|
|
||
| #include <cstdint> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: #includes are not sorted properly [llvm-include-order]
#include <cstdint>
^this fix will not be applied because it overlaps with another fix
test/ramcoretests.cxx
Outdated
|
|
||
| testing::internal::GetCapturedStdout(); | ||
| testing::internal::CaptureStdout(); | ||
| ramview(ttreeFile, region, /*cache=*/true, /*perfstats=*/false, nullptr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: argument comment missing for literal argument 'perfstatsfilename' [bugprone-argument-comment]
| ramview(ttreeFile, region, /*cache=*/true, /*perfstats=*/false, nullptr); | |
| ramview(ttreeFile, region, /*cache=*/true, /*perfstats=*/false, /*perfstatsfilename=*/nullptr); |
test/ramcoretests.cxx
Outdated
| testing::internal::GetCapturedStdout(); | ||
| testing::internal::CaptureStdout(); | ||
| ramview(ttreeFile, region, /*cache=*/true, /*perfstats=*/false, nullptr); | ||
| std::string ramview_output = testing::internal::GetCapturedStdout(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: no header providing "std::string" is directly included [misc-include-cleaner]
test/ramcoretests.cxx:11:
+ #include <string>
test/ramcoretests.cxx
Outdated
| testing::internal::GetCapturedStdout(); | ||
| testing::internal::CaptureStdout(); | ||
| ramview(ttreeFile, region, /*cache=*/true, /*perfstats=*/false, nullptr); | ||
| std::string ramview_output = testing::internal::GetCapturedStdout(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: variable 'ramview_output' is not initialized [cppcoreguidelines-init-variables]
| std::string ramview_output = testing::internal::GetCapturedStdout(); | |
| std::string ramview_output = 0 = testing::internal::GetCapturedStdout(); |
test/ramcoretests.cxx
Outdated
| } | ||
| testing::internal::CaptureStdout(); | ||
| ramntupleview(rntupleFile, region, /*cache=*/true, /*perfstats=*/false, nullptr); | ||
| std::string ramntupleview_output = testing::internal::GetCapturedStdout(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: variable 'ramntupleview_output' is not initialized [cppcoreguidelines-init-variables]
| std::string ramntupleview_output = testing::internal::GetCapturedStdout(); | |
| std::string ramntupleview_output = 0 = testing::internal::GetCapturedStdout(); |
| #include <string> | ||
| #include <cstring> | ||
| #include <vector> | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: included header vector is not used directly [misc-include-cleaner]
tools/samtoramntuple.cxx
Outdated
| do_split = true; | ||
| } else if (arg == "-only") { | ||
| if (i + 1 < argc) { | ||
| only_chromosomes.emplace_back(argv[++i]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
only_chromosomes.emplace_back(argv[++i]);
^There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
There were too many comments to post at once. Showing the first 10 out of 23. Check the log or trigger a new build to see more.
| #include "ramcore/SamParser.h" | ||
| #include "rntuple/RAMNTupleRecord.h" | ||
|
|
||
| #include <cstdint> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: #includes are not sorted properly [llvm-include-order]
#include <cstdint>
^this fix will not be applied because it overlaps with another fix
|
|
||
| #include <cstdint> | ||
| #include <map> | ||
| #include <memory> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: included header map is not used directly [misc-include-cleaner]
| #include <memory> | |
| #include <memory> |
| #include <cstdint> | ||
| #include <map> | ||
| #include <memory> | ||
| #include <cstdio> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: included header memory is not used directly [misc-include-cleaner]
| #include <cstdio> | |
| #include <cstdio> |
src/ramcore/SamToNTuple.cxx
Outdated
| #include <memory> | ||
| #include <cstdio> | ||
| #include <thread> | ||
| #include <vector> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: included header thread is not used directly [misc-include-cleaner]
| #include <vector> | |
| #include <vector> |
| #include <cstdio> | ||
| #include <thread> | ||
| #include <vector> | ||
| #include <mutex> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: included header vector is not used directly [misc-include-cleaner]
| #include <mutex> | |
| #include <mutex> |
| #include <mutex> | ||
| #include <algorithm> | ||
| #include <unordered_set> | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: included header unordered_set is not used directly [misc-include-cleaner]
| bool index, bool split, bool cache, | ||
| int compression_algorithm, | ||
| uint32_t quality_policy) | ||
| void samtoramntuple(const char *datafile, const char *treefile, bool index, bool split, bool cache, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: function 'samtoramntuple' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
| void samtoramntuple(const char *datafile, const char *treefile, bool index, bool split, bool cache, | |
| static void samtoramntuple(const char *datafile, const char *treefile, bool index, bool split, bool cache, |
| const int32_t POS_INTERVAL = 10000; | ||
| const int64_t MAPPED_INTERVAL = 100; | ||
|
|
||
| auto header_callback = [&headers](const std::string &tag, const std::string &content) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: no header providing "std::string" is directly included [misc-include-cleaner]
src/ramcore/SamToNTuple.cxx:9:
- #include <vector>
+ #include <string>
+ #include <vector>| if (sn_pos != std::string::npos) { | ||
| sn_pos += 3; | ||
| size_t tab_pos = content.find('\t', sn_pos); | ||
| std::string ref_name = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: variable 'ref_name' is not initialized [cppcoreguidelines-init-variables]
| std::string ref_name = | |
| std::string ref_name = 0 = |
src/ramcore/SamToNTuple.cxx
Outdated
| }; | ||
|
|
||
| if (!parser.ParseFile(datafile, header_callback, record_callback)) { | ||
| printf("Failed to parse SAM file %s\n", datafile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: do not call c-style vararg functions [cppcoreguidelines-pro-type-vararg]
printf("Failed to parse SAM file %s\n", datafile);
^There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
There were too many comments to post at once. Showing the first 10 out of 18. Check the log or trigger a new build to see more.
| #include <cstdio> | ||
| #include <map> | ||
| #include <memory> | ||
| #include <mutex> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: included header memory is not used directly [misc-include-cleaner]
| #include <mutex> | |
| #include <mutex> |
| #include <mutex> | ||
| #include <string> | ||
| #include <thread> | ||
| #include <unordered_set> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: included header thread is not used directly [misc-include-cleaner]
| #include <unordered_set> | |
| #include <unordered_set> |
| #include <string> | ||
| #include <thread> | ||
| #include <unordered_set> | ||
| #include <vector> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: included header unordered_set is not used directly [misc-include-cleaner]
| #include <vector> | |
| #include <vector> |
| #include <thread> | ||
| #include <unordered_set> | ||
| #include <vector> | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: included header vector is not used directly [misc-include-cleaner]
src/ramcore/SamToNTuple.cxx
Outdated
| bool index, bool split, bool cache, | ||
| int compression_algorithm, | ||
| uint32_t quality_policy) | ||
| void samtoramntuple(const char *datafile, const char *treefile, bool index, bool split, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: function 'samtoramntuple' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
| void samtoramntuple(const char *datafile, const char *treefile, bool index, bool split, | |
| static void samtoramntuple(const char *datafile, const char *treefile, bool index, bool split, |
| if (!std::filesystem::exists("samexample.sam")) { | ||
| GenerateSAMFile("samexample.sam", num_reads_for_test); | ||
| } | ||
| void SetUp() override |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: method 'SetUp' can be made static [readability-convert-member-functions-to-static]
| void SetUp() override | |
| static void SetUp() override |
| std::remove("test_ttree.root"); | ||
| std::remove("test_rntuple.root"); | ||
| } | ||
| void TearDown() override |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: method 'TearDown' can be made static [readability-convert-member-functions-to-static]
| void TearDown() override | |
| static void TearDown() override |
| } | ||
|
|
||
| TEST_F(ramcoreTest, RNTupleView) | ||
| TEST_F(ramcoreTest, ConversionProducesEqualEntries) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: function 'TEST_F' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
| TEST_F(ramcoreTest, ConversionProducesEqualEntries) | |
| TEST_Fstatic (ramcoreTest, ConversionProducesEqualEntries) |
test/ramcoretests.cxx
Outdated
| } | ||
| testing::internal::CaptureStdout(); | ||
| ramview(ttreeFile, region, /*cache=*/true, /*perfstats=*/false, /*perfstatsfilename=*/nullptr); | ||
| std::string ramview_output; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: variable 'ramview_output' is not initialized [cppcoreguidelines-init-variables]
| std::string ramview_output; | |
| std::string ramview_output = 0; |
test/ramcoretests.cxx
Outdated
| ASSERT_NE(reader, nullptr) << "RNTuple file corrupted after viewing"; | ||
| testing::internal::CaptureStdout(); | ||
| ramntupleview(rntupleFile, region, /*cache=*/true, /*perfstats=*/false, /*perfstatsfilename=*/nullptr); | ||
| std::string ramntupleview_output; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: variable 'ramntupleview_output' is not initialized [cppcoreguidelines-init-variables]
| std::string ramntupleview_output; | |
| std::string ramntupleview_output = 0; |
a89b30d to
bcde36d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
tools/samtoramntuple.cxx
Outdated
| int main(int argc, char *argv[]) | ||
| { | ||
| if (argc < 2) { | ||
| std::cout << "Usage: " << argv[0] << " <input.sam> [output]\n"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
std::cout << "Usage: " << argv[0] << " <input.sam> [output]\n";
^
tools/samtoramntuple.cxx
Outdated
| output = argv[i]; | ||
| } | ||
| } | ||
| const char *input = argv[1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
const char *input = argv[1];
^
tools/samtoramntuple.cxx
Outdated
| return 0; | ||
| } | ||
| for (int i = 2; i < argc; i++) { | ||
| std::string arg = argv[i]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
std::string arg = argv[i];
^
tools/samtoramntuple.cxx
Outdated
| } else if (arg == "-only") { | ||
| if (i + 1 < argc) { | ||
|
|
||
| only_chromosomes.emplace_back(argv[++i]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
only_chromosomes.emplace_back(argv[++i]);
^
tools/samtoramntuple.cxx
Outdated
| } | ||
| } else if (arg == "-noindex") { | ||
| do_index = false; | ||
| } else if (arg == "-illumina") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: repeated branch body in conditional chain [bugprone-branch-clone]
} else if (arg == "-illumina") {
^Additional context
tools/samtoramntuple.cxx:44: end of the original
} else if (arg == "-dropqual") {
^tools/samtoramntuple.cxx:44: clone 1 starts here
} else if (arg == "-dropqual") {
^
tools/samtoramntuple.cxx
Outdated
| } else if (arg == "-dropqual") { | ||
| quality_mode = RAMNTupleRecord::kDrop; | ||
| } else if (arg[0] != '-') { | ||
| output = argv[i]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
output = argv[i];
^
tools/samtoramntuple.cxx
Outdated
| } | ||
| } | ||
|
|
||
| std::string outfile; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: variable 'outfile' is not initialized [cppcoreguidelines-init-variables]
| std::string outfile; | |
| std::string outfile = 0; |
| } | ||
| samtoramntuple(input, ramfile.c_str(), do_index, true, true, 505, quality_mode); | ||
| } | ||
| } catch (const std::exception &e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: no header providing "std::exception" is directly included [misc-include-cleaner]
tools/samtoramntuple.cxx:2:
- #include <iostream>
+ #include <exception>
+ #include <iostream>There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
src/ramcore/SamToNTuple.cxx
Outdated
| #include <cstdint> | ||
| #include <cstdio> | ||
| #include <map> | ||
| #include <memory> // NOLINT(misc-include-cleaner) - used for std::unique_ptr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: included header map is not used directly [misc-include-cleaner]
| #include <memory> // NOLINT(misc-include-cleaner) - used for std::unique_ptr | |
| #include <memory> // NOLINT(misc-include-cleaner) - used for std::unique_ptr |
src/ramcore/SamToNTuple.cxx
Outdated
| bool index, bool split, bool cache, | ||
| int compression_algorithm, | ||
| uint32_t quality_policy) | ||
| void samtoramntuple(const char *datafile, const char *treefile, bool index, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: function 'samtoramntuple' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
| void samtoramntuple(const char *datafile, const char *treefile, bool index, | |
| static void samtoramntuple(const char *datafile, const char *treefile, bool index, |
test/ramcoretests.cxx
Outdated
| } | ||
| testing::internal::CaptureStdout(); | ||
| ramview(ttreeFile, region, /*cache=*/true, /*perfstats=*/false, /*perfstatsfilename=*/nullptr); | ||
| std::string ramview_output = testing::internal::GetCapturedStdout(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: variable 'ramview_output' is not initialized [cppcoreguidelines-init-variables]
| std::string ramview_output = testing::internal::GetCapturedStdout(); | |
| std::string ramview_output = 0 = testing::internal::GetCapturedStdout(); |
test/ramcoretests.cxx
Outdated
| ASSERT_NE(reader, nullptr) << "RNTuple file corrupted after viewing"; | ||
| testing::internal::CaptureStdout(); | ||
| ramntupleview(rntupleFile, region, /*cache=*/true, /*perfstats=*/false, /*perfstatsfilename=*/nullptr); | ||
| std::string ramntupleview_output = testing::internal::GetCapturedStdout(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: variable 'ramntupleview_output' is not initialized [cppcoreguidelines-init-variables]
| std::string ramntupleview_output = testing::internal::GetCapturedStdout(); | |
| std::string ramntupleview_output = 0 = testing::internal::GetCapturedStdout(); |
| @@ -1,67 +1,80 @@ | |||
| #include "ramcore/SamToNTuple.h" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: 'ramcore/SamToNTuple.h' file not found [clang-diagnostic-error]
#include "ramcore/SamToNTuple.h"
^
tools/samtoramntuple.cxx
Outdated
| #include <exception> | ||
| #include <iostream> | ||
| #include <string> | ||
| #include <cstring> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: #includes are not sorted properly [llvm-include-order]
| #include <cstring> | |
| #include <cstring> | |
| #include <exception> | |
| #include <iostream> | |
| #include <string> |
tools/samtoramntuple.cxx
Outdated
| int main(int argc, char *argv[]) | ||
| { | ||
| if (argc < 2) { | ||
| std::cout << "Usage: " << argv[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
std::cout << "Usage: " << argv[0]
^
tools/samtoramntuple.cxx
Outdated
| } | ||
| } else if (arg == "-noindex") { | ||
| do_index = false; | ||
| } else if (arg == "-illumina") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: repeated branch body in conditional chain [bugprone-branch-clone]
} else if (arg == "-illumina") {
^There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
| void samtoramntuple(const char *datafile, | ||
| const char *treefile, | ||
| bool index, bool split, bool cache, | ||
| void samtoramntuple(const char *datafile, const char *treefile, bool index, bool split, bool cache, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: function 'samtoramntuple' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
| void samtoramntuple(const char *datafile, const char *treefile, bool index, bool split, bool cache, | |
| static void samtoramntuple(const char *datafile, const char *treefile, bool index, bool split, bool cache, |
test/ramcoretests.cxx
Outdated
| ASSERT_TRUE(ft && !ft->IsZombie()) << "Failed to open TTree file"; | ||
| class ramcoreTest : public ::testing::Test { | ||
| protected: | ||
| const int m_num_reads_for_test = 100; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: member 'm_num_reads_for_test' of type 'const int' is const qualified [cppcoreguidelines-avoid-const-or-ref-data-members]
const int m_num_reads_for_test = 100;
^
test/ramcoretests.cxx
Outdated
| ASSERT_TRUE(ft && !ft->IsZombie()) << "Failed to open TTree file"; | ||
| class ramcoreTest : public ::testing::Test { | ||
| protected: | ||
| const int m_num_reads_for_test = 100; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: member variable 'm_num_reads_for_test' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]
const int m_num_reads_for_test = 100;
^
test/ramcoretests.cxx
Outdated
| class ramcoreTest : public ::testing::Test { | ||
| protected: | ||
| const int m_num_reads_for_test = 100; | ||
| const std::string m_samFile = "samexample.sam"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: member 'm_samFile' of type 'const std::string' (aka 'const int') is const qualified [cppcoreguidelines-avoid-const-or-ref-data-members]
const std::string m_samFile = "samexample.sam";
^
test/ramcoretests.cxx
Outdated
| class ramcoreTest : public ::testing::Test { | ||
| protected: | ||
| const int m_num_reads_for_test = 100; | ||
| const std::string m_samFile = "samexample.sam"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: member variable 'm_samFile' has public visibility [cppcoreguidelines-non-private-member-variables-in-classes]
const std::string m_samFile = "samexample.sam";
^
test/ramcoretests.cxx
Outdated
| protected: | ||
| const int m_num_reads_for_test = 100; | ||
| const std::string m_samFile = "samexample.sam"; | ||
| const std::string m_ttreeFile = "test_ttree.root"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: member 'm_ttreeFile' of type 'const std::string' (aka 'const int') is const qualified [cppcoreguidelines-avoid-const-or-ref-data-members]
const std::string m_ttreeFile = "test_ttree.root";
^
test/ramcoretests.cxx
Outdated
| protected: | ||
| const int m_num_reads_for_test = 100; | ||
| const std::string m_samFile = "samexample.sam"; | ||
| const std::string m_ttreeFile = "test_ttree.root"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: member variable 'm_ttreeFile' has public visibility [cppcoreguidelines-non-private-member-variables-in-classes]
const std::string m_ttreeFile = "test_ttree.root";
^
test/ramcoretests.cxx
Outdated
| const int m_num_reads_for_test = 100; | ||
| const std::string m_samFile = "samexample.sam"; | ||
| const std::string m_ttreeFile = "test_ttree.root"; | ||
| const std::string m_rntupleFile = "test_rntuple.root"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: member 'm_rntupleFile' of type 'const std::string' (aka 'const int') is const qualified [cppcoreguidelines-avoid-const-or-ref-data-members]
const std::string m_rntupleFile = "test_rntuple.root";
^
test/ramcoretests.cxx
Outdated
| const int m_num_reads_for_test = 100; | ||
| const std::string m_samFile = "samexample.sam"; | ||
| const std::string m_ttreeFile = "test_ttree.root"; | ||
| const std::string m_rntupleFile = "test_rntuple.root"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: member variable 'm_rntupleFile' has public visibility [cppcoreguidelines-non-private-member-variables-in-classes]
const std::string m_rntupleFile = "test_rntuple.root";
^Signed-off-by: AdityaPandeyCN <[email protected]> clang changes Signed-off-by: AdityaPandeyCN <[email protected]>
Signed-off-by: AdityaPandeyCN <[email protected]>
f6e9e94 to
f737701
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
test/ramcoretests.cxx
Outdated
| samtoram(samFile, ttreeFile, true, true, true, 1, 0); | ||
| samtoramntuple(samFile, rntupleFile, true, true, true, 505, 0); | ||
| constexpr int kNumReadsForTest = 100; | ||
| constexpr const char kSamFile[] = "samexample.sam"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays]
constexpr const char kSamFile[] = "samexample.sam";
^
test/ramcoretests.cxx
Outdated
| samtoramntuple(samFile, rntupleFile, true, true, true, 505, 0); | ||
| constexpr int kNumReadsForTest = 100; | ||
| constexpr const char kSamFile[] = "samexample.sam"; | ||
| constexpr const char kTTreeFile[] = "test_ttree.root"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays]
constexpr const char kTTreeFile[] = "test_ttree.root";
^
test/ramcoretests.cxx
Outdated
| constexpr int kNumReadsForTest = 100; | ||
| constexpr const char kSamFile[] = "samexample.sam"; | ||
| constexpr const char kTTreeFile[] = "test_ttree.root"; | ||
| constexpr const char kRNTupleFile[] = "test_rntuple.root"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays]
constexpr const char kRNTupleFile[] = "test_rntuple.root";
^
test/ramcoretests.cxx
Outdated
| auto reader = ROOT::RNTupleReader::Open("RAM", rntupleFile); | ||
| ASSERT_NE(reader, nullptr) << "Failed to open RNTuple"; | ||
| Long64_t rntupleEntries = reader->GetNEntries(); | ||
| std::remove(kTTreeFile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: do not implicitly decay an array into a pointer; consider using gsl::array_view or an explicit cast instead [cppcoreguidelines-pro-bounds-array-to-pointer-decay]
std::remove(kTTreeFile);
^
test/ramcoretests.cxx
Outdated
| ASSERT_NE(reader, nullptr) << "Failed to open RNTuple"; | ||
| Long64_t rntupleEntries = reader->GetNEntries(); | ||
| std::remove(kTTreeFile); | ||
| std::remove(kRNTupleFile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: do not implicitly decay an array into a pointer; consider using gsl::array_view or an explicit cast instead [cppcoreguidelines-pro-bounds-array-to-pointer-decay]
std::remove(kRNTupleFile);
^
test/ramcoretests.cxx
Outdated
| } | ||
| void TearDown() override | ||
| { | ||
| std::remove(kTTreeFile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: do not implicitly decay an array into a pointer; consider using gsl::array_view or an explicit cast instead [cppcoreguidelines-pro-bounds-array-to-pointer-decay]
std::remove(kTTreeFile);
^
test/ramcoretests.cxx
Outdated
| void TearDown() override | ||
| { | ||
| std::remove(kTTreeFile); | ||
| std::remove(kRNTupleFile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: do not implicitly decay an array into a pointer; consider using gsl::array_view or an explicit cast instead [cppcoreguidelines-pro-bounds-array-to-pointer-decay]
std::remove(kRNTupleFile);
^
test/ramcoretests.cxx
Outdated
|
|
||
| testing::internal::GetCapturedStdout(); | ||
| testing::internal::CaptureStdout(); | ||
| ramview(kTTreeFile, region, /*cache=*/true, /*perfstats=*/false, /*perfstatsfilename=*/nullptr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: do not implicitly decay an array into a pointer; consider using gsl::array_view or an explicit cast instead [cppcoreguidelines-pro-bounds-array-to-pointer-decay]
ramview(kTTreeFile, region, /*cache=*/true, /*perfstats=*/false, /*perfstatsfilename=*/nullptr);
^Signed-off-by: AdityaPandeyCN <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
| #include <Rtypes.h> | ||
| #include <string> | ||
| #include <string_view> | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: included header string_view is not used directly [misc-include-cleaner]
Signed-off-by: AdityaPandeyCN <[email protected]>
Signed-off-by: AdityaPandeyCN <[email protected]>
|
LGTM! |
This PR addresses:-