Fix cppcoreguidelines-narrowing-conversions warning reported by clang-tidy #1159

MaximSmolskiy · 2022-12-04T21:56:13Z

PragmaTwice · 2022-12-05T10:44:59Z

src/server/redis_reply.h

+template <typename IntegerType>
+std::string Integer(IntegerType data);


Actually you can implement this template function in the header directly to avoid explicit instantiations.

…ons-warning-reported-by-clang-tidy

src/types/redis_zset.cc

…ons-warning-reported-by-clang-tidy

torwig · 2022-12-07T06:36:59Z

src/common/string_util.cc

@@ -121,7 +121,7 @@ int StringMatch(const std::string &pattern, const std::string &in, int nocase) {
 }

 // Glob-style pattern matching.
-int StringMatchLen(const char *pattern, int patternLen, const char *string, int stringLen, int nocase) {
+int StringMatchLen(const char *pattern, size_t patternLen, const char *string, size_t stringLen, int nocase) {


Since I noticed it via your PR, you can change the camel case to the snake case here for function arguments.

src/config/config.h

…ons-warning-reported-by-clang-tidy

PragmaTwice · 2022-12-10T13:58:00Z

src/config/config_type.h

+  }
+  Status Set(const std::string &v) override {
+    int64_t n;
+    auto s = Util::DecimalStringToNum(v, &n, min_, max_);


I think you can use ParseInt<uint32_t>(v, {min_, max_}) here.

PragmaTwice · 2022-12-10T14:59:03Z

src/config/config_type.h

  std::string ToString() override { return std::to_string(*receiver_); }
  Status ToNumber(int64_t *n) override {
    *n = *receiver_;
    return Status::OK();
  }
  Status Set(const std::string &v) override {
-    int64_t n;
-    auto s = Util::DecimalStringToNum(v, &n, min_, max_);
+    auto s = ParseInt(v, {min_, max_});


Suggested change

auto s = ParseInt(v, {min_, max_});

auto s = ParseInt<IntegerType>(v, {min_, max_});

It can be deduced. Is it better to explicitly pass type?

I think it is better to pass type explicitly here.

PragmaTwice · 2022-12-10T15:11:56Z

src/config/config.cc

 #ifdef ENABLE_OPENSSL
-      {"tls-port", true, new IntField(&tls_port, 0, 0, PORT_LIMIT)},
+      {"tls-port", true, new IntegerField(&tls_port, (uint32_t)0, (uint32_t)0, PORT_LIMIT)},


Suggested change

{"tls-port", true, new IntegerField(&tls_port, (uint32_t)0, (uint32_t)0, PORT_LIMIT)},

{"tls-port", true, new IntegerField<uint32_t>(&tls_port, 0, 0, PORT_LIMIT)},

I prefer we specify template types explicitly instead of deduced from arguments.

In all places (and with int type) or only in few places with types uint32_t and uint64_t?

I think you can define some aliases, e.g. using IntField = IntergerField<int>, using UInt32Field = IntergerField<uint32_t>.

Then you can use these alias in the config map, so most config kv will remain the original form (IntField).

torwig

LGTM

git-hulk · 2022-12-12T02:32:04Z

Thanks all, merging...

Empty

245e299

MaximSmolskiy marked this pull request as draft December 4, 2022 21:57

MaximSmolskiy added 11 commits December 5, 2022 01:03

Fix src/stats/log_collector.cc

0084655

Fix clang-diagnostic-error

01539d3

Fix clang-diagnostic-error

9daa79f

Fix src/common/io_util.cc

a2fe597

Fix src/common/string_util.cc

60cc348

Fix src/common/io_util.cc

0e3275e

Fix

1f46891

Fix format

10461c6

Fix errors

187d5a1

Fix

fec58b1

Fix

1e4d846

PragmaTwice reviewed Dec 5, 2022

View reviewed changes

MaximSmolskiy added 16 commits December 5, 2022 14:05

Fix

af2f01a

Fix

6f63f41

Fix src/storage/lock_manager.cc

d7410bf

Fix src/main.cc

501be08

Fix src/config/config.cc

a05a728

Fix src/server/redis_connection.cc

54b7842

Fix src/server/redis_request.cc

82cb120

Fix src/server/worker.cc

291e577

Fix src/storage/storage.cc

32aea24

Fix src/storage/storage.cc

27ef7a2

Fix src/commands/redis_cmd.cc

9818f10

Fix src/server/server.cc

dbbc259

Fix src/storage/scripting.cc

d752b38

Fix src/types/geohash.cc

91f3286

Fix src/types/redis_bitmap_string.cc

3127679

Fix src/storage/scripting.cc

3e7333c

MaximSmolskiy marked this pull request as ready for review December 5, 2022 22:04

MaximSmolskiy requested a review from PragmaTwice December 5, 2022 22:05

Merge branch 'unstable' into fix-cppcoreguidelines-narrowing-conversi…

dcabf0d

…ons-warning-reported-by-clang-tidy

torwig reviewed Dec 6, 2022

View reviewed changes

src/types/redis_zset.cc Show resolved Hide resolved

Merge branch 'unstable' into fix-cppcoreguidelines-narrowing-conversi…

a45b195

…ons-warning-reported-by-clang-tidy

MaximSmolskiy requested review from torwig and PragmaTwice and removed request for PragmaTwice and torwig December 6, 2022 18:11

torwig reviewed Dec 7, 2022

View reviewed changes

MaximSmolskiy added 3 commits December 10, 2022 14:47

Merge branch 'unstable' into fix-cppcoreguidelines-narrowing-conversi…

f01cf83

…ons-warning-reported-by-clang-tidy

Change the camel case to the snake case

810ef4c

Change port and tls_port types from int to uint32_t

8cdb044

MaximSmolskiy requested a review from torwig December 10, 2022 13:50

PragmaTwice reviewed Dec 10, 2022

View reviewed changes

Add templated IntegerField

77758b6

PragmaTwice reviewed Dec 10, 2022

View reviewed changes

Specify types

a5ec5f2

MaximSmolskiy requested review from PragmaTwice and removed request for torwig December 10, 2022 16:12

torwig approved these changes Dec 10, 2022

View reviewed changes

PragmaTwice approved these changes Dec 11, 2022

View reviewed changes

PragmaTwice requested a review from git-hulk December 11, 2022 13:44

git-hulk approved these changes Dec 11, 2022

View reviewed changes

git-hulk merged commit e6aa898 into apache:unstable Dec 12, 2022

MaximSmolskiy deleted the fix-cppcoreguidelines-narrowing-conversions-warning-reported-by-clang-tidy branch December 12, 2022 04:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix cppcoreguidelines-narrowing-conversions warning reported by clang-tidy #1159

Fix cppcoreguidelines-narrowing-conversions warning reported by clang-tidy #1159

MaximSmolskiy commented Dec 4, 2022 •

edited by PragmaTwice

Loading

PragmaTwice Dec 5, 2022

torwig Dec 7, 2022

MaximSmolskiy Dec 10, 2022

PragmaTwice Dec 10, 2022 •

edited

Loading

PragmaTwice Dec 10, 2022

MaximSmolskiy Dec 10, 2022 •

edited

Loading

PragmaTwice Dec 10, 2022

MaximSmolskiy Dec 10, 2022

PragmaTwice Dec 10, 2022

MaximSmolskiy Dec 10, 2022

PragmaTwice Dec 10, 2022 •

edited

Loading

MaximSmolskiy Dec 10, 2022

torwig left a comment

git-hulk commented Dec 12, 2022

		template <typename IntegerType>
		std::string Integer(IntegerType data);

	auto s = ParseInt(v, {min_, max_});
	auto s = ParseInt<IntegerType>(v, {min_, max_});

	{"tls-port", true, new IntegerField(&tls_port, (uint32_t)0, (uint32_t)0, PORT_LIMIT)},
	{"tls-port", true, new IntegerField<uint32_t>(&tls_port, 0, 0, PORT_LIMIT)},

Fix cppcoreguidelines-narrowing-conversions warning reported by clang-tidy #1159

Fix cppcoreguidelines-narrowing-conversions warning reported by clang-tidy #1159

Conversation

MaximSmolskiy commented Dec 4, 2022 • edited by PragmaTwice Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PragmaTwice Dec 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaximSmolskiy Dec 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PragmaTwice Dec 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torwig left a comment

Choose a reason for hiding this comment

git-hulk commented Dec 12, 2022

MaximSmolskiy commented Dec 4, 2022 •

edited by PragmaTwice

Loading

PragmaTwice Dec 10, 2022 •

edited

Loading

MaximSmolskiy Dec 10, 2022 •

edited

Loading

PragmaTwice Dec 10, 2022 •

edited

Loading