Page MenuHomePhabricator

[clang-format] [PR39719] clang-format converting object-like macro to function-like macro
AbandonedPublic

Authored by MyDeveloperDay on Apr 6 2019, 5:41 AM.

Details

Summary

Clang format is incorrectly formatting macros where keywords are redefined

See https://bugs.llvm.org/show_bug.cgi?id=39719

The following code

- #define true ((foo)1)
- #define false ((foo)0)

becomes

+ #define true((foo)1)
+ #define false((foo)0)

Diff Detail

Event Timeline

MyDeveloperDay created this revision.Apr 6 2019, 5:41 AM
owenpan requested changes to this revision.Apr 7 2019, 11:52 PM
owenpan added inline comments.
clang/lib/Format/TokenAnnotator.cpp
2467–2470

I think it can be more precise and simplified to something like this:

if (Left.Previous && Left.Previous->is(tok::pp_define) &&
    Left.isNot(tok::identifier) && Right.is(tok::l_paren))
This revision now requires changes to proceed.Apr 7 2019, 11:52 PM
klimek added inline comments.Apr 8 2019, 1:08 AM
clang/lib/Format/TokenAnnotator.cpp
2467–2470

Why don't we have the same problem for identifier? Is that already solved and the problem is that this is a keyword redefinition?

MyDeveloperDay marked 2 inline comments as done.Apr 8 2019, 2:00 AM
MyDeveloperDay added inline comments.
clang/lib/Format/TokenAnnotator.cpp
2467–2470

Yes the identifier seems to work ok, but when its a keyword redfinition the identifier is replaced with the token for the keyword i.e. tok::kw_true or tok::kw_false

klimek added inline comments.Apr 8 2019, 2:04 AM
clang/lib/Format/TokenAnnotator.cpp
2467–2470

And the idea is that for non-ID
#define true(x) x
won't work anyway? (otherwise this patch would be incorrect, right?)

Have you looked at where we detect the diff between
#define a(x) x
and
#define a (x)
in the identifier case and looked we could add common keyword macro cases there?

MyDeveloperDay planned changes to this revision.Apr 8 2019, 5:48 AM
MyDeveloperDay marked 2 inline comments as done.
MyDeveloperDay added inline comments.
clang/lib/Format/TokenAnnotator.cpp
2467–2470

I see what you mean, this path will reformat the false #define incorrectly

#define true ((foo)1)
#define false(x) x

will be transformed to

#define true ((foo)1)
#define false (x) x
lebedev.ri set the repository for this revision to rC Clang.Apr 8 2019, 6:23 AM
lebedev.ri edited projects, added Restricted Project; removed Restricted Project.
lebedev.ri edited subscribers, added: cfe-commits; removed: llvm-commits.
klimek added inline comments.Apr 8 2019, 8:14 AM
clang/lib/Format/TokenAnnotator.cpp
2467–2470

Exactly.

@klimek one possible solution to this might be to replace the "keyword" back to an identifier in a '#define <keywoord>' scenario

Maybe something like this?

bool FormatTokenLexer::tryConvertKeyWordDefines() {
  // ensure #define keyword x = tok::hash,tok::identifier,tok::identifier
  if (Tokens.size() < 3)
    return false;

  auto &Hash = *(Tokens.end() - 3);
  auto &Define = *(Tokens.end() - 2);
  auto &Keyword = *(Tokens.end() - 1);

  if (!Hash->is(tok::hash))
    return false;

  if (!Define->is(tok::identifier))
    return false;

  // Already an identifier
  if (Keyword->is(tok::identifier))
    return false;

  if (!Define->Tok.getIdentifierInfo() ||
      Define->Tok.getIdentifierInfo()->getPPKeywordID() != tok::pp_define)
    return false;

  // switch the type to be an identifier
  Keyword->Tok.setKind(tok::identifier);
  return true;
}

A more straightforward way, IMO, is to add to the spaceRequiredBetween function a separate if statement that returns false for the sequence of tokens: #, define, tok::identifier, and (

Actually, there is a neater way: https://reviews.llvm.org/D60853