[PATCH] D129570: [clang-tidy] Add new clang-tidy check to find implicit conversions from enum to integer.

pfultz2 updated this revision to Diff 488445. pfultz2 added a comment. Merge from main. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129570/new/ https://reviews.llvm.org/D129570 Files: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp

[PATCH] D129570: [clang-tidy] Add new clang-tidy check to find implicit conversions from enum to integer.

pfultz2 updated this revision to Diff 488440. pfultz2 added a comment. Improve null checking, use the correct type in the fixit, and updated the tests. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129570/new/ https://reviews.llvm.org/D129570 Files:

[PATCH] D129570: [clang-tidy] Add new clang-tidy check to find implicit conversions from enum to integer.

pfultz2 added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/EnumToIntCheck.cpp:45 + } else { +Note << FixItHint::CreateInsertion(MatchedExpr->getBeginLoc(), "(int)"); + } carlosgalvezp wrote: > I don't think we can assume the type of

[PATCH] D129570: [clang-tidy] Add new clang-tidy check to find implicit conversions from enum to integer.

pfultz2 added a comment. I would like to see this merged in. Is there anyone available to review or approve? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129570/new/ https://reviews.llvm.org/D129570

[PATCH] D129570: [clang-tidy] Add new clang-tidy check to find implicit conversions from enum to integer.

pfultz2 added a comment. @njames93 I fixed the review comments, can this be merged? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129570/new/ https://reviews.llvm.org/D129570 ___ cfe-commits mailing

[PATCH] D129570: [clang-tidy] Add new clang-tidy check to find implicit conversions from enum to integer.

pfultz2 added a comment. All review comments have been addressed. I assume this can be merged now. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129570/new/ https://reviews.llvm.org/D129570 ___

[PATCH] D129570: [clang-tidy] Add new clang-tidy check to find implicit conversions from enum to integer.

pfultz2 added a comment. So can this be merged now? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129570/new/ https://reviews.llvm.org/D129570 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D129570: [clang-tidy] Add new clang-tidy check to find implicit conversions from enum to integer.

pfultz2 added a comment. Anymore feedback? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129570/new/ https://reviews.llvm.org/D129570 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D129570: [clang-tidy] Add new clang-tidy check to find implicit conversions from enum to integer.

pfultz2 updated this revision to Diff 454632. pfultz2 added a comment. Updated to diagnose `return` statements as well. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129570/new/ https://reviews.llvm.org/D129570 Files:

[PATCH] D129570: [clang-tidy] Add new clang-tidy check to find implicit conversions from enum to integer.

pfultz2 updated this revision to Diff 453515. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129570/new/ https://reviews.llvm.org/D129570 Files: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt

[PATCH] D129570: [clang-tidy] Add new clang-tidy check to find implicit conversions from enum to integer.

pfultz2 updated this revision to Diff 453514. pfultz2 added a comment. Update review comments and added fixit hints. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129570/new/ https://reviews.llvm.org/D129570 Files:

[PATCH] D129570: [clang-tidy] Add new clang-tidy check to find implicit conversions from enum to integer.

pfultz2 added a comment. And for context, here is an actual bug this check would help find: https://github.com/ROCmSoftwarePlatform/MIOpen/pull/1578#discussion_r889038610 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129570/new/ https://reviews.llvm.org/D129570

[PATCH] D129570: [clang-tidy] Add new clang-tidy check to find implicit conversions from enum to integer.

pfultz2 added a comment. > What is the motivation for requiring these to be the arguments of call or > construct expressions? It is to just to try and limit the possible false positives at first. Usually a function call it is not clear locally that it will be converted to an integer but

[PATCH] D129570: [clang-tidy] Add new clang-tidy check to find implicit conversions from enum to integer.

pfultz2 added a comment. Anymore feedback? Can this be merged now? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129570/new/ https://reviews.llvm.org/D129570 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D129570: Add new clang-tidy check to find implicit conversions from enum to integer.

pfultz2 updated this revision to Diff 451666. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129570/new/ https://reviews.llvm.org/D129570 Files: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt

[PATCH] D129570: Add new clang-tidy check to find implicit conversions from enum to integer.

pfultz2 updated this revision to Diff 451284. pfultz2 added a comment. Fix typo and merge from main. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129570/new/ https://reviews.llvm.org/D129570 Files:

[PATCH] D129570: Add new clang-tidy check to find implicit conversions from enum to integer.

pfultz2 created this revision. pfultz2 added reviewers: aaron.ballman, alexfh. pfultz2 added a project: clang-tools-extra. Herald added subscribers: carlosgalvezp, mgorny. Herald added a project: All. pfultz2 requested review of this revision. Herald added a subscriber: cfe-commits. This check

[PATCH] D46081: [analyzer] Expand conversion check to check more expressions for overflow and underflow

pfultz2 added a comment. Thanks for merging as I dont have commit access. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D46081/new/ https://reviews.llvm.org/D46081 ___ cfe-commits mailing list

[PATCH] D78655: [CUDA][HIP] Let lambda be host device by default

pfultz2 added inline comments. Comment at: clang/test/SemaCUDA/lambda.cu:27 + +kernel<<<1,1>>>([&](){ hd(b); }); +// dev-error@-1 {{capture host side class data member by this pointer in device or host device lambda function}} Will this still produce

[PATCH] D78655: [CUDA][HIP] Let non-caputuring lambda be host device

pfultz2 added a comment. > What's the expected HD property of this template function clip? I think it is intended to be host-only. The function `f` will launch a kernel or threads to utilize the passed lambda. Ideally, it would be nice to make all inlineable functions implicitly HD. There is

[PATCH] D78655: [CUDA][HIP] Let non-caputuring lambda be host device

pfultz2 added a comment. > Now, back to the specifics of your example. I'm still not 100% sure I > understand what the problem is. Can you boil down the use case to an example > on godbolt? I dont have a specific example, but there could be code like this generic `clip` operator: template

[PATCH] D78655: [CUDA][HIP] Let non-caputuring lambda be host device

pfultz2 added a comment. > Could you give an example to demonstrate current use and how it will break? Here is place where it would break: https://github.com/ROCmSoftwarePlatform/AMDMIGraphX/blob/develop/src/targets/gpu/device/include/migraphx/gpu/device/multi_index.hpp#L129 This change was

[PATCH] D78655: [CUDA][HIP] Let non-caputuring lambda be host device

pfultz2 added inline comments. Comment at: clang/lib/Sema/SemaCUDA.cpp:753 return; + if (LI.Default == LCD_None && LI.Captures.size() == 0) { +Method->addAttr(CUDADeviceAttr::CreateImplicit(Context)); There should at least be a flag to enable capturing

[PATCH] D78655: [HIP] Add -fhip-lambda-host-device

pfultz2 added a comment. > A slight variation on that, that might be better: lambdas with no > lambda-capture are implicitly HD; lambdas with any lambda-capture (which must > therefore have an enclosing function) inherit the enclosing function's HDness. Lambdas should already inherit the

[PATCH] D78655: [HIP] Add -fhip-lambda-host-device

pfultz2 added a comment. Is it possible to add a test like this? kernel<<<1,1>>>([=](){ auto f = [&]{ hd(); }; f(); }); That should not have a compiler error. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78655/new/ https://reviews.llvm.org/D78655

[PATCH] D78655: [HIP] Let lambda be host device by default

pfultz2 added a comment. > It captures addresses as seen at the point in time on the processor which > executed the constructor. Yea and the same happens when assigning the address to a pointer, which is later used on a different device. > Another point that capturing lambdas are not

[PATCH] D78655: [HIP] Let lambda be host device by default

pfultz2 added a comment. > I.e. if I pass a mutable lambda by reference to the GPU kernel I dont think we are enabling passing host objects by reference through functions. Although it could be possible to capture the mutable lambda by reference by another lambda. > will the same lambda called

[PATCH] D78655: [HIP] Let lambda be host device by default

pfultz2 added a comment. > Not only the capture is an issue, like a regular function, lambda could also > access non-local variables/functions. In practice this is not an issue. Hcc will implictly treat anything inlinable as host device, and user's are not confused or surprised when they use

[PATCH] D78655: [HIP] Let lambda be host device by default

pfultz2 added a subscriber: AlexVlx. pfultz2 added a comment. > says we capture host variable reference in a device lambda. Is that required to be an error? I know @AlexVlx added support to hcc at one point to capture host variables by reference. So it seems to be possible for it to work

[PATCH] D78655: [HIP] Let lambda be host device by default

pfultz2 added inline comments. Comment at: clang/lib/Sema/SemaCUDA.cpp:723 +Method->addAttr(CUDADeviceAttr::CreateImplicit(Context)); +Method->addAttr(CUDAHostAttr::CreateImplicit(Context)); +return; Shouldn't we add these attributes if there are no

[PATCH] D52281: [clang-tidy] Add modernize check to use std::invoke in generic code

pfultz2 added a comment. This needs a test when calling in a `constexpr` function. I believe `std::invoke` is not `constepxr`, so a function object call in a `constexpr` function should not suggest `std::invoke`. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52281

[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers

pfultz2 updated this revision to Diff 146719. pfultz2 added a comment. Rebased https://reviews.llvm.org/D46159 Files: clang-tidy/ClangTidy.cpp clang-tidy/ClangTidy.h clang-tidy/ClangTidyDiagnosticConsumer.cpp clang-tidy/ClangTidyDiagnosticConsumer.h clang-tidy/tool/ClangTidyMain.cpp

[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers

pfultz2 added a comment. Is someone able to merge in my changes? https://reviews.llvm.org/D46159 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers

pfultz2 updated this revision to Diff 145925. pfultz2 added a comment. Some changes based on feedback. https://reviews.llvm.org/D46159 Files: clang-tidy/ClangTidy.cpp clang-tidy/ClangTidy.h clang-tidy/ClangTidyDiagnosticConsumer.cpp clang-tidy/ClangTidyDiagnosticConsumer.h

[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers

pfultz2 added a comment. > In this sense bug reports against abandoned alpha checkers (which are, > unfortunately, the majority) aren't very useful. In most cases it's just too > easy to see how broken they are. Although the majority are like that, not all of them are. Some like the

[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers

pfultz2 updated this revision to Diff 145229. pfultz2 added a comment. Moved flag for alpha checks to the ClangTidyContext https://reviews.llvm.org/D46159 Files: clang-tidy/ClangTidy.cpp clang-tidy/ClangTidy.h clang-tidy/ClangTidyDiagnosticConsumer.cpp

[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers

pfultz2 added a comment. There doesn't seem to be a simple way to remove it from the ClangTidyOptions class, as a lot more functions need to be changed to support that. I would prefer to leave it there for now, and we can refactor it later. Either way, the check can't be set from a config

[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers

pfultz2 updated this revision to Diff 145072. pfultz2 added a comment. Rename flag to `AllowEnablingAnalyzerAlphaCheckers`. https://reviews.llvm.org/D46159 Files: clang-tidy/ClangTidy.cpp clang-tidy/ClangTidyOptions.cpp clang-tidy/ClangTidyOptions.h clang-tidy/tool/ClangTidyMain.cpp

[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers

pfultz2 added inline comments. Comment at: clang-tidy/ClangTidyOptions.h:80-82 + /// \brief Turns on experimental alpha checkers from the static analyzer. + llvm::Optional AllowEnablingAlphaChecks; + alexfh wrote: > Since this will only be configurable via a

[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers

pfultz2 added a comment. > When something is merged into Clang trunk, the expectation is that it will be > production quality or will be worked on rapidly to get it to production > quality, which is somewhat orthogonal to getting user feedback. I don't know > that I have the full history of

[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers

pfultz2 updated this revision to Diff 145029. pfultz2 added a comment. Rename flag to `allow-enabling-alpha-checks` and removed the option from the clang-tidy file. https://reviews.llvm.org/D46159 Files: clang-tidy/ClangTidy.cpp clang-tidy/ClangTidyOptions.cpp

[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers

pfultz2 added a comment. > I think the premise is a bit off the mark. It's not that these are not for > the common user -- it's that they're simply not ready for users at all. Then why was it merged into clang in the first place? It seems like the whole point of merging it into clang is to get

[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers

pfultz2 added a comment. > But still, could you explain the use case and why a local modification of > clang-tidy is not an option? Because I would like to direct users to try an alpha check on thier local codebases without having to tell them to rebuild clang. Repository: rCTE Clang Tools

[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers

pfultz2 added inline comments. Comment at: clang-tidy/ClangTidy.cpp:373-376 // FIXME: Remove this option once clang's cfg-temporary-dtors option defaults // to true. AnalyzerOptions->Config["cfg-temporary-dtors"] = Context.getOptions().AnalyzeTemporaryDtors ?

[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers

pfultz2 added a comment. > As Devin says (and as we discussed this with Anna Zaks) alpha checkers are > still in development, so we don't want to expose them to the users, even very > curious ones. Then why do we make them available with `clang --analyze`? If the plan is not to expose them to

[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers

pfultz2 added a comment. > Do you have some better choices? I could do `-allow-alpha-checks`. What do you think? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D46159 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers

pfultz2 added a comment. Do you want the flag to be renamed? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D46159 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers

pfultz2 added a comment. > Am i understanding correctly that the new -enable-alpha-checks flag doesn't > enable alpha checks, but it only allows enabling them with a separate command? Yes, it enables them to be selected by clang tidy. Repository: rCTE Clang Tools Extra

[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers

pfultz2 added a comment. > I'm curious about the use case for this, since I don't think it ever makes > sense to run all the alpha checks at once. These checks are typically a work > in progress (they're not finished yet) and often have false positives that > haven't been fixed yet. We want

[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers

pfultz2 created this revision. pfultz2 added reviewers: aaron.ballman, hokein, ilya-biryukov. Herald added subscribers: cfe-commits, xazax.hun. The alpha checkers can already be enabled using the clang driver, this allows them to be enabled using the clang-tidy as well. This can make it easier

[PATCH] D46081: [analyzer] Expand conversion check to check more expressions for overflow and underflow

pfultz2 updated this revision to Diff 144202. pfultz2 added a comment. So I ran this on clang/llvm code base and fixed some false positives. https://reviews.llvm.org/D46081 Files: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp test/Analysis/conversion.c test/Analysis/conversion.cpp

[PATCH] D46081: [analyzer] Expand conversion check to check more expressions for overflow and underflow

pfultz2 added a comment. So moving to core will require explicit casts in some of the tests, especially for things like: `memcpy(a262.s1, input, -1)`. Or this could be moved to another section instead of core. In https://reviews.llvm.org/D45532 there is talk of adding a bugprone section. I

[PATCH] D46081: [analyzer] Expand conversion check to check more expressions for overflow and underflow

pfultz2 added a comment. > Have you tried on any large codebases? This check is not available to the user yet. I can move it to core if you want. Repository: rC Clang https://reviews.llvm.org/D46081 ___ cfe-commits mailing list

[PATCH] D46081: [analyzer] Expand conversion check to check more expressions for overflow and underflow

pfultz2 added inline comments. Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:77 if (Opc == BO_Assign) { - LossOfSign = isLossOfSign(Cast, C); - LossOfPrecision = isLossOfPrecision(Cast, Cast->getType(), C); + if

[PATCH] D46081: [analyzer] Expand conversion check to check more expressions for overflow and underflow

pfultz2 added a comment. > While I understand extending the analyzer to cover more is a good approach, > there is -Wconversion which seemingly covers this -- or at least the trivial > case(?): Yes, but it won't catch something like this, which is more interesting: void g(unsigned); void

[PATCH] D46066: [analyzer] Add checker for underflowing unsigned integers

pfultz2 abandoned this revision. pfultz2 added a comment. So I submitted my change to the ConversionChecker, instead. Repository: rC Clang https://reviews.llvm.org/D46066 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D46081: [analyzer] Expand conversion check to check more expressions for overflow and underflow

pfultz2 created this revision. pfultz2 added reviewers: NoQ, xazax.hun, dkrupp, whisperity. pfultz2 added a project: clang. Herald added subscribers: cfe-commits, a.sidorin, rnkovacs, szepet. Herald added a reviewer: george.karpenkov. This expands checking for more expressions. This will check

[PATCH] D46066: [analyzer] Add checker for underflowing unsigned integers

pfultz2 added a comment. > Isn't this case already covered by conversion checker? I was unaware of this. This looks like it only works for binary operators. So `f(-1)` won't get caught. Repository: rC Clang https://reviews.llvm.org/D46066 ___

[PATCH] D46066: [analyzer] Add checker for underflowing unsigned integers

pfultz2 created this revision. pfultz2 added reviewers: NoQ, xazax.hun, dkrupp, whisperity, george.karpenkov. pfultz2 added a project: clang. Herald added subscribers: cfe-commits, a.sidorin, rnkovacs, szepet, mgorny. This will check for when assigning a negative value to an unsigned integer,

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

pfultz2 added a comment. This seems like it would be nice to have in `bugprone-*` as well. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D38455 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D44231: [clang-tidy] Check for sizeof that call functions

pfultz2 updated this revision to Diff 140790. pfultz2 added a comment. I have rebased. https://reviews.llvm.org/D44231 Files: clang-tidy/bugprone/SizeofExpressionCheck.cpp clang-tidy/bugprone/SizeofExpressionCheck.h docs/clang-tidy/checks/bugprone-sizeof-expression.rst

[PATCH] D44231: [clang-tidy] Check for sizeof that call functions

pfultz2 added a comment. Is someone able to merge in my changes? https://reviews.llvm.org/D44231 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44231: [clang-tidy] Check for sizeof that call functions

pfultz2 added a comment. > Do you need someone to submit this for you? Yes https://reviews.llvm.org/D44231 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44231: [clang-tidy] Check for sizeof that call functions

pfultz2 added a comment. So, I've added tests for class enums and bols. https://reviews.llvm.org/D44231 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44231: [clang-tidy] Check for sizeof that call functions

pfultz2 updated this revision to Diff 139649. https://reviews.llvm.org/D44231 Files: clang-tidy/misc/SizeofExpressionCheck.cpp clang-tidy/misc/SizeofExpressionCheck.h docs/clang-tidy/checks/misc-sizeof-expression.rst test/clang-tidy/misc-sizeof-expression.cpp Index:

[PATCH] D44231: [clang-tidy] Check for sizeof that call functions

pfultz2 marked an inline comment as done. pfultz2 added a comment. I have reworded the documentation. Hopefully it is clearer. https://reviews.llvm.org/D44231 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D44231: [clang-tidy] Check for sizeof that call functions

pfultz2 added a comment. > As this patch can catch some mistakes, I'm fine with checking it in. I agree > that it is reasonable to write normal code like sizeof(func_call()) (not > false positive), maybe set the option to false by default? I have disabled it by default. We can decide later if

[PATCH] D44231: [clang-tidy] Check for sizeof that call functions

pfultz2 updated this revision to Diff 138791. https://reviews.llvm.org/D44231 Files: clang-tidy/misc/SizeofExpressionCheck.cpp clang-tidy/misc/SizeofExpressionCheck.h docs/clang-tidy/checks/misc-sizeof-expression.rst test/clang-tidy/misc-sizeof-expression.cpp Index:

[PATCH] D44231: [clang-tidy] Check for sizeof that call functions

pfultz2 added a comment. So I've updated the documentation on this. Are there any other updates needed for this to get it merged in? https://reviews.llvm.org/D44231 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D44231: [clang-tidy] Check for sizeof that call functions

pfultz2 updated this revision to Diff 137822. https://reviews.llvm.org/D44231 Files: clang-tidy/misc/SizeofExpressionCheck.cpp clang-tidy/misc/SizeofExpressionCheck.h docs/clang-tidy/checks/misc-sizeof-expression.rst test/clang-tidy/misc-sizeof-expression.cpp Index:

[PATCH] D44231: [clang-tidy] Check for sizeof that call functions

pfultz2 added a comment. > I don't have a script for it. I've used "bear" with at least some of those > projects because they use makefiles rather than cmake > (https://github.com/rizsotto/Bear). I'm not tied to those projects > specifically, either, so if you have a different corpus you'd

[PATCH] D44231: [clang-tidy] Check for sizeof that call functions

pfultz2 added a comment. > Again, that only works for C++ and not C. Typedef has always worked in C. > Did it report any true positives that would need correcting? Not for LLVM, but it has in other projects like I mentioned. > Can you check some other large repos (both C++ and C), such as:

[PATCH] D44231: [clang-tidy] Check for sizeof that call functions

pfultz2 added a comment. > If the return type of foo() is changed, then the use for s1 will be > automatically updated But usually you write it as: using foo_return = uint16_t; foo_return foo(); ... size_t s1 = sizeof(foo()); size_t s2 = sizeof(foo_return); So you just update the

[PATCH] D44231: [clang-tidy] Check for sizeof that call functions

pfultz2 added a comment. > Can you point to some real world code that would benefit from this check? Yes, here: https://github.com/ROCmSoftwarePlatform/MIOpen/blob/master/src/convolution.cpp#L184 It is erroneously calling `sizeof(yDesc.GetType())`. The `GetType` function returns an enum that

[PATCH] D44241: [clang-tidy] Add a check for constructing non-trivial types without assigning to a variable

pfultz2 added a comment. > How does this differ from misc-unused-raii? Actually, I was unaware of this check. This seems like a better place to start. Whats missing from the misc-unused-raii is that it doesn't catch it when constructing with curly braces(ie `NonTrivial{}`). Let me work on

[PATCH] D44231: [clang-tidy] Check for sizeof that call functions

pfultz2 added a comment. > Can you elaborate a bit more about this? This catches problems when calling `sizeof(f())` when `f` returns an integer(or enum). This is because, most likely, the integer represents the type to be chosen, however, `sizeof` is being computed for an integer and not the

[PATCH] D44241: Add a check for constructing non-trivial types without assigning to a variable

pfultz2 created this revision. pfultz2 added reviewers: clang-tools-extra, aaron.ballman. pfultz2 added a project: clang-tools-extra. Herald added a subscriber: mgorny. Diagnoses when a non-trivial type is not assigned to a variable. This is useful to check for problems like unnamed lock guards.

[PATCH] D44231: Check for sizeof that call functions

pfultz2 updated this revision to Diff 137499. https://reviews.llvm.org/D44231 Files: clang-tidy/misc/SizeofExpressionCheck.cpp clang-tidy/misc/SizeofExpressionCheck.h test/clang-tidy/misc-sizeof-expression.cpp Index: test/clang-tidy/misc-sizeof-expression.cpp

[PATCH] D44231: Check for sizeof that call functions

pfultz2 updated this revision to Diff 137497. https://reviews.llvm.org/D44231 Files: clang-tidy/misc/SizeofExpressionCheck.cpp clang-tidy/misc/SizeofExpressionCheck.h test/clang-tidy/misc-sizeof-expression.cpp Index: test/clang-tidy/misc-sizeof-expression.cpp

[PATCH] D44231: Check for sizeof that call functions

pfultz2 updated this revision to Diff 137496. https://reviews.llvm.org/D44231 Files: clangd/CodeComplete.cpp test/clangd/protocol.test unittests/clangd/CodeCompleteTests.cpp Index: unittests/clangd/CodeCompleteTests.cpp ===

[PATCH] D44231: Check for sizeof that call functions

pfultz2 created this revision. pfultz2 added reviewers: clang-tools-extra, hokein, alexfh, aaron.ballman, ilya-biryukov. Herald added a subscriber: jkorous-apple. A common mistake that I have found in our codebase is calling a function to get an integer or enum that represents the type such as: