lib/crash.cpp: Some checking (bounds etc. via the new `nelems` and `azzert`). #3297

DoctorNoobingstoneIPresume · 2022-05-09T06:06:56Z

No description provided.

onf-cla-manager · 2022-05-09T06:07:05Z

Hi @DoctorNoobingstoneIPresume, this is the ONF bot 🤖 I'm glad you want to contribute to our projects! However, before accepting your contribution, we need to ask you to sign a Contributor License Agreement (CLA). You can do it online, it will take only a few minutes:

✒️ 👉 https://cla.opennetworking.org

After signing, make sure to add your Github user ID DoctorNoobingstoneIPresume to the agreement.

For more information or help:"
https://wiki.opennetworking.org/x/BgCUI

mihaibudiu · 2022-05-09T17:30:40Z

lib/util.h

+// TODO: We should only allow `a` to be an array (i.e. we should rule out pointers).
+
+#ifndef nelems
+ #define nelems(a) (sizeof (a) / sizeof (0 [a]))


we have a definition of this macro in algorithms.h too.

we have a definition of this macro in algorithms.h too.

Thank you. I am seeing it now:

#define ELEMENTS(a) (sizeof(a) / sizeof(a[0]))

I think it should be (in order to support non-trivial argument in macro invocation):

#define ELEMENTS(a) (sizeof(a) / sizeof((a)[0]))

or better (in order to prevent a from being an object with user-defined operator[], e.g. std::vector <...> or std::deque <...>, which would not give what the user probably expects):

#define ELEMENTS(a) (sizeof(a) / sizeof(0[a]))

Note an even better implementation (for C 11 and above) is just:
template<typename t, size_t i>
constexpr inline size_t sizeofarray(const t (&arr)[i]) { return i; }

No reasons for macros.

Note for C 17 , std::size works too.
See https://en.cppreference.com/w/cpp/iterator/size .

Note an even better implementation (for C 11 and above) is just: template<typename t, size_t i> constexpr inline size_t sizeofarray(const t (&arr)[i]) { return i; }

No reasons for macros.

Thank you.

I believe your suggestion is also safer, preventing errors such as invoking the macro not with (a reference to) an array, but instead with (a reference to) a pointer (possibly after array-to-pointer conversion, as for argument-to-parameter when calling a function).

mihaibudiu · 2022-05-09T17:31:59Z

lib/util.h

+[[noreturn]] void azzertionHasFailed (const char *pszFile, unsigned iLine, const char *pszFunction, const char *pszExpression, const char *pszMessage = nullptr);
+
+#ifndef azzert
+ #define azzert(b) do { if (! (b)) azzertionHasFailed (__FILE__, __LINE__, __PRETTY_FUNC__, #e); } while (0)


I find "util.h" obscuring the contents of this file. This should be in either in error.h or in a file called "azzert.h".

…azzert`. `azzert` is generally useful to bring internal compiler errors closer to bugs and to self-document code expectancies (in an easy-to-`grep`-for form).

mihaibudiu · 2022-05-10T17:49:03Z

Even if you don't like our cpplint rules, your code still has to pass the check to be merged.
You can submit a PR against cpplint if you feel strongly about it, it's just a python program.

DoctorNoobingstoneIPresume · 2022-05-13T01:49:06Z

Even if you don't like our cpplint rules, your code still has to pass the check to be merged. You can submit a PR against cpplint if you feel strongly about it, it's just a python program.

Thank you for the suggestion (also appliable to my other pull requests (#3296, #3295, #3294).

FWTAW (for whaterver they are worth), my current thoughts are:

If one adds a new source file, a different style might be allowed (if acceptable to the other contributors).
If I simply add a line inside an existing function, it is probably better to change it to match the style of the other lines. I intend to modify my PR's as such.
Generally, I feel there is a scale to allowing different style: (1) extra line(s) in existing function; (2) extra function(s) in existing class/namespace/file; (3) extra class in existing namespace/file; (4) extra file in existing folder; (5) extra folder in current project/repo; ... (9) external application (using the compiler) written on planet in different solar system (but still targetting x86, of course); ... (19) ... ARM and other architectures...

Regarding the order of #included headers, even if other persons end up agreeing with #3295 (comment), I have thought and currently I feel that for existing #include directives, it may be better to keep them as they are, in order not to affect too much the output of (some common incantations of) the git log -p, git blame etc. -- but instead allow different orders (if agreed upon with regard to advantages) for new #include directives (at least in new files).

=> We currently might not want to impose different constraints on existing code, but instead to relax constraints or modify them for new code.

Currently, I think we have // NOLINT comment to suppress cpplint for one line.
We might wish to support something like // NOLINT (WhiteSpace, IncludeOrder) { and // NOLINT } (yes, with opening brace and closing brace) in order to suppress (certain options of) cpplint for a larger block.

I intend to learn how to do that, but I need some time (really to learn the basics of Python).

In the meanwhile, I think opinions from others will be very good (I am just a new person here, with ~zero contribution). E.g.: "Yes, sometimes the style constraints are too restrictive and parts of CodingStandardPhilosophy.md are subjective." or "No, the current order of headers is better because..." or "A better way to suppress cpplint might be..." or "Be gone, Intruder !!".

mihaibudiu · 2022-05-13T02:00:14Z

Contributions are appreciated, but I have too much other stuff to do to fight with cpplint.
You can use // NOLINT indeed.

fruffy · 2022-05-13T02:18:53Z

Part of the reason these linters exist is for style consistency and to offload some of the review work to automated tools. I think we should have even more linters, particularly ones that enforce code formatting. I might add clang-format at some point. We have been using it successfully for other P4C projects.

Afaik the order this project uses is defined by the Google style guide. I personally also prefer the LLVM style, but this repo has committed to this particular guideline.

Even then, I have not encountered a case where the include order restriction really causes a substantial problem. If there really is a case where the include order matters, we can always use NOLINT. However, usually these scenarios indicate that there is something funky going on.

DoctorNoobingstoneIPresume · 2022-05-16T10:04:12Z

Contributions are appreciated, but I have too much other stuff to do to fight with cpplint. You can use // NOLINT indeed.

Thank you. IIUC, we have //NOLINT for individual lines, but not for blocks (i.e. groups of lines). I hope to be able to implement that.

DoctorNoobingstoneIPresume · 2022-05-16T10:09:42Z

Part of the reason these linters exist is for style consistency and to offload some of the review work to automated tools. I think we should have even more linters, particularly ones that enforce code formatting. I might add clang-format at some point. We have been using it successfully for other P4C projects.

Automating such tasks is good !

Afaik the order this project uses is defined by the Google style guide. I personally also prefer the LLVM style, but this repo has committed to this particular guideline.

I have not found the rationale behind the #include order in the Google Style guide.

FWIW, I have given my rationale for the different order: #3295 (comment).

Even then, I have not encountered a case where the include order restriction really causes a substantial problem. If there really is a case where the include order matters, we can always use NOLINT. However, usually these scenarios indicate that there is something funky going on.

Let us suppose we want to modify the behaviour of libgc headers (or the headers of other dependency) by some #define -- and we place those #define-itions in a project-specific header that we expect our source code to #include before/instead-of #include-ing the libgc etc. header.

This is not going to work smoothly because of the current order of #include's -- i.e. other files in our project might #include the dependency header before the #define-ing header.

Also the self-sufficiency of headers is less checked with the current order of #include's.

(I am sorry for repeating myself.)

fruffy · 2022-05-17T01:21:28Z

I have not found the rationale behind the #include order in the Google Style guide.

Yeah I do not think there is one provided. Afaik the reason is a) consistent style b) to throw an error when you redefine something in a local file (since you can change those).

Regarding your example, the easiest solution it to likely mandate to always include the customized include header instead of the gc system header. Or to have the #define directive be explicit with every include.

Personally I am not opposed to changing the include style. In fact, I would not mind getting rid of cpplint and using the better-maintained clang-tidy instead.

However, there does not seem sufficient justification for that right now. Does #3295 not compile if you move the include up?

DoctorNoobingstoneIPresume · 2022-05-20T10:25:03Z

Thank you, @fruffy. Partial answer to just your last question:

However, there does not seem sufficient justification for that right now. Does #3295 not compile if you move the include up?

I agree. I believe it does compile, however I am going to check exactly and get back soon !

DoctorNoobingstoneIPresume force-pushed the 220509_lib_crash_01h branch from b619009 to b512fd5 Compare May 9, 2022 07:53

mihaibudiu requested changes May 9, 2022

View reviewed changes

lib/crash.cpp: Checking (bounds, single init). Signal number shown. `…

6805db2

…azzert`. `azzert` is generally useful to bring internal compiler errors closer to bugs and to self-document code expectancies (in an easy-to-`grep`-for form).

DoctorNoobingstoneIPresume force-pushed the 220509_lib_crash_01h branch from b512fd5 to 6805db2 Compare May 10, 2022 11:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib/crash.cpp: Some checking (bounds etc. via the new `nelems` and `azzert`). #3297

lib/crash.cpp: Some checking (bounds etc. via the new `nelems` and `azzert`). #3297

DoctorNoobingstoneIPresume commented May 9, 2022

onf-cla-manager bot commented May 9, 2022 •

edited

Loading

mihaibudiu May 9, 2022

DoctorNoobingstoneIPresume May 10, 2022 •

edited

Loading

apinski-cavium May 10, 2022

apinski-cavium May 10, 2022

DoctorNoobingstoneIPresume May 10, 2022 •

edited

Loading

mihaibudiu May 9, 2022

mihaibudiu commented May 10, 2022

DoctorNoobingstoneIPresume commented May 13, 2022 •

edited

Loading

mihaibudiu commented May 13, 2022

fruffy commented May 13, 2022

DoctorNoobingstoneIPresume commented May 16, 2022

DoctorNoobingstoneIPresume commented May 16, 2022 •

edited

Loading

fruffy commented May 17, 2022

DoctorNoobingstoneIPresume commented May 20, 2022

lib/crash.cpp: Some checking (bounds etc. via the new nelems and azzert). #3297

Are you sure you want to change the base?

lib/crash.cpp: Some checking (bounds etc. via the new nelems and azzert). #3297

Conversation

DoctorNoobingstoneIPresume commented May 9, 2022

onf-cla-manager bot commented May 9, 2022 • edited Loading

mihaibudiu May 9, 2022

Choose a reason for hiding this comment

DoctorNoobingstoneIPresume May 10, 2022 • edited Loading

Choose a reason for hiding this comment

apinski-cavium May 10, 2022

Choose a reason for hiding this comment

apinski-cavium May 10, 2022

Choose a reason for hiding this comment

DoctorNoobingstoneIPresume May 10, 2022 • edited Loading

Choose a reason for hiding this comment

mihaibudiu May 9, 2022

Choose a reason for hiding this comment

mihaibudiu commented May 10, 2022

DoctorNoobingstoneIPresume commented May 13, 2022 • edited Loading

mihaibudiu commented May 13, 2022

fruffy commented May 13, 2022

DoctorNoobingstoneIPresume commented May 16, 2022

DoctorNoobingstoneIPresume commented May 16, 2022 • edited Loading

fruffy commented May 17, 2022

DoctorNoobingstoneIPresume commented May 20, 2022

lib/crash.cpp: Some checking (bounds etc. via the new `nelems` and `azzert`). #3297

lib/crash.cpp: Some checking (bounds etc. via the new `nelems` and `azzert`). #3297

onf-cla-manager bot commented May 9, 2022 •

edited

Loading

DoctorNoobingstoneIPresume May 10, 2022 •

edited

Loading

DoctorNoobingstoneIPresume May 10, 2022 •

edited

Loading

DoctorNoobingstoneIPresume commented May 13, 2022 •

edited

Loading

DoctorNoobingstoneIPresume commented May 16, 2022 •

edited

Loading