[all-commits] [llvm/llvm-project] 0685e8: Fix cppcoreguidelines-virtual-base-class-destructo...

[Balazs Benics via All-commits]

  Branch: refs/heads/main
  Home:   https://github.com/llvm/llvm-project
  Commit: 0685e83534ef8917f277b394da2927cabff8129f
      https://github.com/llvm/llvm-project/commit/0685e83534ef8917f277b394da2927cabff8129f
  Author: Balazs Benics <balazs.benics at sigmatechnology.se>
  Date:   2021-11-29 (Mon, 29 Nov 2021)

  Changed paths:
    M clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
    M clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp

  Log Message:
  -----------
  Fix cppcoreguidelines-virtual-base-class-destructor in macros

The `cppcoreguidelines-virtual-base-class-destructor` checker crashed on
this example:

  #define DECLARE(CLASS) \
  class CLASS {          \
  protected:             \
    virtual ~CLASS();    \
  }
  DECLARE(Foo); // no-crash

The checker will hit the following assertion:

  clang-tidy: llvm/include/llvm/ADT/Optional.h:196: T &llvm::optional_detail::OptionalStorage<clang::Token, true>::getValue() & [T = clang::Token]: Assertion `hasVal' failed."

It turns out, `Lexer::findNextToken()` returned `llvm::None` within the
`getVirtualKeywordRange()` function when the `VirtualEndLoc`
SourceLocation represents a macro expansion.
To prevent this from happening, I decided to propagate the `llvm::None`
further up and only create the removal of `virtual` if the
`getVirtualKeywordRange()` succeeds.

I considered an alternative fix for this issue:
I could have checked the `Destructor.getLocation().isMacroID()` before
doing any Fixit calculation inside the `check()` function.
In contrast to this approach my patch will preserve the diagnostics and
drop the fixits only if it would have crashed.

Reviewed By: whisperity

Differential Revision: https://reviews.llvm.org/D113558


  Commit: 0540485436c4dd225e6a40e6db1240f096d145d3
      https://github.com/llvm/llvm-project/commit/0540485436c4dd225e6a40e6db1240f096d145d3
  Author: Balazs Benics <balazs.benics at sigmatechnology.se>
  Date:   2021-11-29 (Mon, 29 Nov 2021)

  Changed paths:
    M clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
    M clang-tools-extra/test/clang-tidy/infrastructure/export-diagnostics.cpp
    M clang-tools-extra/unittests/clang-tidy/ClangTidyDiagnosticConsumerTest.cpp

  Log Message:
  -----------
  [libtooling][clang-tidy] Fix crashing on rendering invalid SourceRanges

Invalid SourceRanges can occur generally if the code does not compile,
thus we expect clang error diagnostics.
Unlike `clang`, `clang-tidy` did not swallow invalid source ranges, but
tried to highlight them, and blow various assertions.

The following two examples produce invalid source ranges, but this is
not a complete list:

  void test(x); // error: unknown type name 'x'
  struct Foo {
    member; // error: C++ requires a type specifier for all declarations
  };

Thanks @whisperity helping me fix this.

Reviewed-By: xazax.hun

Differential Revision: https://reviews.llvm.org/D114254


  Commit: e1d0673aeeece138d4865385a24a86f6954dff72
      https://github.com/llvm/llvm-project/commit/e1d0673aeeece138d4865385a24a86f6954dff72
  Author: Balazs Benics <balazs.benics at sigmatechnology.se>
  Date:   2021-11-29 (Mon, 29 Nov 2021)

  Changed paths:
    M clang-tools-extra/clang-tidy/altera/StructPackAlignCheck.cpp
    A clang-tools-extra/test/clang-tidy/checkers/altera-struct-pack-align-invalid-decl-no-crash.cpp

  Log Message:
  -----------
  [clang-tidy] Fix crashing altera-struct-pack-align on invalid RecordDecls

Reviewed-By: martong

Differential Revision: https://reviews.llvm.org/D114256


  Commit: a8120a771143c15480b508c19a14c0c85a36378c
      https://github.com/llvm/llvm-project/commit/a8120a771143c15480b508c19a14c0c85a36378c
  Author: Balazs Benics <balazs.benics at sigmatechnology.se>
  Date:   2021-11-29 (Mon, 29 Nov 2021)

  Changed paths:
    M clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
    A clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-bitfields.cpp

  Log Message:
  -----------
  [clang-tidy] Ignore narrowing conversions in case of bitfields

Bitfields are special. Due to integral promotion [conv.prom/5] bitfield
member access expressions are frequently wrapped by an implicit cast to
`int` if that type can represent all the values of the bitfield.

Consider these examples:
  struct SmallBitfield { unsigned int id : 4; };
  x.id & 1;             (case-1)
  x.id & 1u;            (case-2)
  x.id << 1u;           (case-3)
  (unsigned)x.id << 1;  (case-4)

Due to the promotion rules, we would get a warning for case-1. It's
debatable how useful this is, but the user at least has a convenient way
of //fixing// it by adding the `u` unsigned-suffix to the literal as
demonstrated by case-2. However, this won't work for shift operators like
the one in case-3. In case of a normal binary operator, both operands
contribute to the result type. However, the type of the shift expression is
the promoted type of the left operand. One could still suppress this
superfluous warning by explicitly casting the bitfield member access as
case-4 demonstrates, but why? The compiler already knew that the value from
the member access should safely fit into an `int`, why do we have this
warning in the first place? So, hereby we suppress this specific scenario,
when a bitfield's value is implicitly cast to int (likely due to integral
promotion).

Note that the bitshift operation might invoke unspecified/undefined
behavior, but that's another topic, this checker is about detecting
conversion-related defects.

Example AST for `x.id << 1`:
  BinaryOperator 'int' '<<'
  |-ImplicitCastExpr 'int' <IntegralCast>
  | `-ImplicitCastExpr 'unsigned int' <LValueToRValue>
  |   `-MemberExpr 'unsigned int' lvalue bitfield .id
  |     `-DeclRefExpr 'SmallBitfield' lvalue ParmVar 'x' 'SmallBitfield'
  `-IntegerLiteral 'int' 1

Reviewed By: courbet

Differential Revision: https://reviews.llvm.org/D114105


Compare: https://github.com/llvm/llvm-project/compare/a31f4bdfe821...a8120a771143