[PATCH] D75012: [ReleaseNotes] Mention -fmacro-prefix-map and -ffile-prefix-map.

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn updated this revision to Diff 246413.
Lekensteyn marked 2 inline comments as done.
Lekensteyn added a comment.

Updated the comment based on @MaskRay's feedback, thanks!
This (and the previous) patch were based on release/10.x, I plan to commit it 
later today since the final tag is tomorrow.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75012/new/

https://reviews.llvm.org/D75012

Files:
  clang/docs/ReleaseNotes.rst


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -138,6 +138,12 @@
   please let us know if you encounter a situation where you need to specify 
this
   flag for correct program behavior.
 
+- ``-fmacro-prefix-map=OLD=NEW`` substitutes directory prefix ``OLD`` for
+  ``NEW`` in predefined preprocessor macros such as ``__FILE__``. This helps
+  with reproducible builds that are location independent. The new
+  ``-ffile-prefix-map`` option is equivalent to specifying both
+  ``-fdebug-prefix-map`` and ``-fmacro-prefix-map``.
+
 Deprecated Compiler Flags
 -
 


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -138,6 +138,12 @@
   please let us know if you encounter a situation where you need to specify this
   flag for correct program behavior.
 
+- ``-fmacro-prefix-map=OLD=NEW`` substitutes directory prefix ``OLD`` for
+  ``NEW`` in predefined preprocessor macros such as ``__FILE__``. This helps
+  with reproducible builds that are location independent. The new
+  ``-ffile-prefix-map`` option is equivalent to specifying both
+  ``-fdebug-prefix-map`` and ``-fmacro-prefix-map``.
+
 Deprecated Compiler Flags
 -
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75012: [ReleaseNotes] Mention -fmacro-prefix-map and -ffile-prefix-map.

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn created this revision.
Lekensteyn added reviewers: dankm, MaskRay, hans.
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D75012

Files:
  clang/docs/ReleaseNotes.rst


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -138,6 +138,12 @@
   please let us know if you encounter a situation where you need to specify 
this
   flag for correct program behavior.
 
+- ``-fmacro-prefix-map=OLD=NEW`` allows a directory prefix ``OLD`` in
+  ``__FILE__`` preprocessor macros to be substituted for ``NEW``. This helps
+  with reproducible builds that are location independent. The new
+  ``-ffile-prefix-map`` option is equivalent to specifying both
+  ``-fdebug-prefix-map`` and ``-fmacro-prefix-map``.
+
 Deprecated Compiler Flags
 -
 


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -138,6 +138,12 @@
   please let us know if you encounter a situation where you need to specify this
   flag for correct program behavior.
 
+- ``-fmacro-prefix-map=OLD=NEW`` allows a directory prefix ``OLD`` in
+  ``__FILE__`` preprocessor macros to be substituted for ``NEW``. This helps
+  with reproducible builds that are location independent. The new
+  ``-ffile-prefix-map`` option is equivalent to specifying both
+  ``-fdebug-prefix-map`` and ``-fmacro-prefix-map``.
+
 Deprecated Compiler Flags
 -
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49466: Initial implementation of -fmacro-prefix-map and -ffile-prefix-map

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn added a comment.

Thanks for picking this up again. I've left some nitpicks below in a quick 
review.

The "strict" parameter is not precisely defined, if that is fixed I think this 
would be ready for merge.




Comment at: clang/test/Driver/debug-prefix-map.c:8
+// RUN: %clang -### -ffile-prefix-map=old=new %s 2>&1 | FileCheck %s 
-check-prefix CHECK-DEBUG-SIMPLE
+// RUN: %clang -### -ffile-prefix-map=old=new %s 2>&1 | FileCheck %s 
-check-prefix CHECK-MACRO-SIMPLE
+

What about combining these two tests? The command is the same, maybe you could 
have a new `-check-prefix` to reduce the number of invocations? Likewise for 
the cases below.



Comment at: llvm/include/llvm/Support/Path.h:172
+/// @param style The path separator style
+/// @param strict Strict prefix path checking
+/// @result true if \a Path begins with OldPrefix

"strict checking" is ambiguous on its own. What about something like:

If strict is true, a directory separator following \a OldPrefix will also 
be stripped. Otherwise, directory separators will only be matched and stripped 
when present in \a OldPrefix.

Or whatever semantics you would like to assign to "strict mode".



Comment at: llvm/include/llvm/Support/Path.h:181
+  return replace_path_prefix(Path, OldPrefix, NewPrefix, style, strict);
+}
 

Why have a variant with the parameters swapped, is it common in LLVM to have 
such convenience wrappers?

Why not require callers to pass `Style::native` whenever they want to modify 
"strict"?



Comment at: llvm/lib/Support/Path.cpp:512
+  if (!strict && OldPrefix.size() > OrigPath.size())
+return false;
+

this condition is duplicated above


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D49466/new/

https://reviews.llvm.org/D49466



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61838: [Sema] Suppress additional warnings for C's zero initializer

[Peter Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL366163: [Sema] Suppress additional warnings for Cs 
zero initializer (authored by Lekensteyn, committed by ).
Herald added a project: LLVM.

Changed prior to commit:
  https://reviews.llvm.org/D61838?vs=209994=210009#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61838/new/

https://reviews.llvm.org/D61838

Files:
  cfe/trunk/lib/AST/Expr.cpp
  cfe/trunk/test/Sema/zero-initializer.c


Index: cfe/trunk/lib/AST/Expr.cpp
===
--- cfe/trunk/lib/AST/Expr.cpp
+++ cfe/trunk/lib/AST/Expr.cpp
@@ -2303,11 +2303,11 @@
 bool InitListExpr::isIdiomaticZeroInitializer(const LangOptions ) 
const {
   assert(isSyntacticForm() && "only test syntactic form as zero initializer");
 
-  if (LangOpts.CPlusPlus || getNumInits() != 1) {
+  if (LangOpts.CPlusPlus || getNumInits() != 1 || !getInit(0)) {
 return false;
   }
 
-  const IntegerLiteral *Lit = dyn_cast(getInit(0));
+  const IntegerLiteral *Lit = 
dyn_cast(getInit(0)->IgnoreImplicit());
   return Lit && Lit->getValue() == 0;
 }
 
Index: cfe/trunk/test/Sema/zero-initializer.c
===
--- cfe/trunk/test/Sema/zero-initializer.c
+++ cfe/trunk/test/Sema/zero-initializer.c
@@ -7,6 +7,8 @@
 struct B { struct A a; };
 struct C { struct B b; };
 struct D { struct C c; int n; };
+struct E { short e; };
+struct F { struct E e; int n; };
 
 int main(void)
 {
@@ -23,6 +25,9 @@
   struct C p = { 0 }; // no-warning
   struct C q = { 9 }; // warning suppressed for struct with single element
   struct D r = { 9 }; // expected-warning {{suggest braces around 
initialization of subobject}} expected-warning {{missing field 'n' initializer}}
+  struct F s = { 0 }; // no-warning
+  struct F t = { 9 }; // expected-warning {{suggest braces around 
initialization of subobject}} expected-warning {{missing field 'n' initializer}}
+
   f = (struct foo ) { 0 }; // no-warning
   g = (struct foo ) { 9 }; // expected-warning {{missing field 'y' 
initializer}}
   h = (struct foo ) { 9, 9 }; // no-warning
@@ -36,6 +41,8 @@
   p = (struct C) { 0 }; // no-warning
   q = (struct C) { 9 }; // warning suppressed for struct with single element
   r = (struct D) { 9 }; // expected-warning {{suggest braces around 
initialization of subobject}} expected-warning {{missing field 'n' initializer}}
+  s = (struct F) { 0 }; // no-warning
+  t = (struct F) { 9 }; // expected-warning {{suggest braces around 
initialization of subobject}} expected-warning {{missing field 'n' initializer}}
 
   return 0;
 }


Index: cfe/trunk/lib/AST/Expr.cpp
===
--- cfe/trunk/lib/AST/Expr.cpp
+++ cfe/trunk/lib/AST/Expr.cpp
@@ -2303,11 +2303,11 @@
 bool InitListExpr::isIdiomaticZeroInitializer(const LangOptions ) const {
   assert(isSyntacticForm() && "only test syntactic form as zero initializer");
 
-  if (LangOpts.CPlusPlus || getNumInits() != 1) {
+  if (LangOpts.CPlusPlus || getNumInits() != 1 || !getInit(0)) {
 return false;
   }
 
-  const IntegerLiteral *Lit = dyn_cast(getInit(0));
+  const IntegerLiteral *Lit = dyn_cast(getInit(0)->IgnoreImplicit());
   return Lit && Lit->getValue() == 0;
 }
 
Index: cfe/trunk/test/Sema/zero-initializer.c
===
--- cfe/trunk/test/Sema/zero-initializer.c
+++ cfe/trunk/test/Sema/zero-initializer.c
@@ -7,6 +7,8 @@
 struct B { struct A a; };
 struct C { struct B b; };
 struct D { struct C c; int n; };
+struct E { short e; };
+struct F { struct E e; int n; };
 
 int main(void)
 {
@@ -23,6 +25,9 @@
   struct C p = { 0 }; // no-warning
   struct C q = { 9 }; // warning suppressed for struct with single element
   struct D r = { 9 }; // expected-warning {{suggest braces around initialization of subobject}} expected-warning {{missing field 'n' initializer}}
+  struct F s = { 0 }; // no-warning
+  struct F t = { 9 }; // expected-warning {{suggest braces around initialization of subobject}} expected-warning {{missing field 'n' initializer}}
+
   f = (struct foo ) { 0 }; // no-warning
   g = (struct foo ) { 9 }; // expected-warning {{missing field 'y' initializer}}
   h = (struct foo ) { 9, 9 }; // no-warning
@@ -36,6 +41,8 @@
   p = (struct C) { 0 }; // no-warning
   q = (struct C) { 9 }; // warning suppressed for struct with single element
   r = (struct D) { 9 }; // expected-warning {{suggest braces around initialization of subobject}} expected-warning {{missing field 'n' initializer}}
+  s = (struct F) { 0 }; // no-warning
+  t = (struct F) { 9 }; // expected-warning {{suggest braces around initialization of subobject}} expected-warning {{missing field 'n' initializer}}
 
   return 0;
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[PATCH] D61838: [Sema] Suppress additional warnings for C's zero initializer

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn accepted this revision.
Lekensteyn added a comment.

Thanks, I'll push once the build and test pass.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61838/new/

https://reviews.llvm.org/D61838



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61838: [Sema] Suppress additional warnings for C's zero initializer

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn added a comment.

@al3xtjames I was about to commit this but noticed that some others check 
whether `getInit(0)` is NULL or not before proceeding. Should that be done here 
as well? If not, why?

See for example "Harden InitListExpr::isStringLiteralInit() against getInit() 
returning null." in
https://github.com/llvm/llvm-project/commit/256bd96d1c2464b0f0ecb482ca52075a3158f6a1#diff-dac09655ff6a54658c320a28a6ea297c
and InitListExpr::isTransparent in
https://github.com/llvm/llvm-project/commit/122f88d481971b68d05ba12395c3b73ab4b31ab3#diff-dac09655ff6a54658c320a28a6ea297c


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61838/new/

https://reviews.llvm.org/D61838



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49466: Initial implementation of -fmacro-prefix-map and -ffile-prefix-map

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn added a comment.

Hi @dankm, any progress on this feature? The proposed branch off date for Clang 
9.0.0 is 18 July 2019: 
https://lists.llvm.org/pipermail/cfe-dev/2019-June/062628.html


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D49466/new/

https://reviews.llvm.org/D49466



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61838: [Sema] Suppress additional warnings for C's zero initializer

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn added a comment.

@rsmith The latest patch version has addressed your feedback, can you confirm 
that this is ready to be merged?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61838/new/

https://reviews.llvm.org/D61838



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61838: [Sema] Suppress additional warnings for C's zero initializer

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn added a comment.

@rsmith Are you happy with the changes, is it ready to be merged?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61838/new/

https://reviews.llvm.org/D61838



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61838: [Sema] Suppress additional warnings for C's zero initializer

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn accepted this revision.
Lekensteyn added a comment.

Verified again!


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61838/new/

https://reviews.llvm.org/D61838



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61838: [Sema] Suppress additional warnings for C's zero initializer

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn accepted this revision.
Lekensteyn added a comment.
This revision is now accepted and ready to land.

Thanks, I have also verified this patch against the above test case.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61838/new/

https://reviews.llvm.org/D61838



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61838: [Sema] Suppress additional warnings for C's zero initializer

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn added a comment.

This looks reasonable to fix the problem at hand, but would it handle nested 
structures too?

  struct s1 {
 short f1;  // "int f1" is fine.
  };
  struct s2 {
 struct s1 f2;
 int x;
  };
  struct s3 {
 struct s2 f3;
 int x;
  };
  struct s2 x1 = {0};
  struct s3 x2 = {0};

produces this AST (`clang-check -ast-dump x.c --`):

  |-VarDecl 0x55e7bbc60390  col:11 x1 'struct s2':'struct 
s2' cinit
  | `-InitListExpr 0x55e7bbc60498  'struct s2':'struct s2'
  |   |-InitListExpr 0x55e7bbc604e8  'struct s1':'struct s1'
  |   | `-ImplicitCastExpr 0x55e7bbc60530  'short' 
  |   |   `-IntegerLiteral 0x55e7bbc60430  'int' 0
  |   `-ImplicitValueInitExpr 0x55e7bbc60548 <> 'int'
  |-VarDecl 0x55e7bbc605b0  col:11 x2 'struct s3':'struct 
s3' cinit
  | `-InitListExpr 0x55e7bbc60678  'struct s3':'struct s3'
  |   |-InitListExpr 0x55e7bbc606c8  'struct s2':'struct s2'
  |   | |-InitListExpr 0x55e7bbc60718  'struct s1':'struct s1'
  |   | | `-ImplicitCastExpr 0x55e7bbc60760  'short' 
  |   | |   `-IntegerLiteral 0x55e7bbc60610  'int' 0
  |   | `-ImplicitValueInitExpr 0x55e7bbc60778 <> 'int'
  |   `-ImplicitValueInitExpr 0x55e7bbc60788 <> 'int'


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61838/new/

https://reviews.llvm.org/D61838



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49466: Initial implementation of -fmacro-prefix-map and -ffile-prefix-map

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn added inline comments.



Comment at: lib/CodeGen/CGDebugInfo.cpp:607
   llvm::DIFile *CUFile = DBuilder.createFile(
-  remapDIPath(MainFileName), remapDIPath(getCurrentDirname()), CSInfo,
+  MainFileName, remapDIPath(getCurrentDirname()), CSInfo,
   getSource(SM, SM.getMainFileID()));

dankm wrote:
> Lekensteyn wrote:
> > Any reason for dropping `remapDIPath` here? Wouldn't this result in the 
> > full path being included even when using:
> > 
> > clang -fdebug-prefix-map=/full/path/= /full/path/source.c
> Whoops. That probably shouldn't have been included this round. This is a 
> bugfix. MainFileName is already remapped from earlier in this function, this 
> keeps it from remapping twice if you have an empty old prefix.
The remapping was done here:
```c
  std::string MainFileDir;
  if (const FileEntry *MainFile = SM.getFileEntryForID(SM.getMainFileID())) {
MainFileDir = remapDIPath(MainFile->getDir()->getName());
```

(Observation: the declaration could probably be moved inside the `if` block 
since it is not used outside.)

What about the second case though? For example, assume `/tmp/testdir/mytest.ii`:
```
# 1 "/tmp/mytest.c"
# 1 ""
# 1 ""
# 31 ""
# 1 "/usr/include/stdc-predef.h" 1 3 4
# 32 "" 2
# 1 "/tmp/mytest.c"
int main(int argc, const char *argv[])
{
return 0;
}
```

What happens if you now compile with `clang -fdebug-prefix-map=/tmp/=/bla/ 
/tmp/testdir/mytest.ii` from `/tmp/testdir`?

Unless this affects the current patch, consider moving it to a separate change.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D49466/new/

https://reviews.llvm.org/D49466



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49466: Initial implementation of -fmacro-prefix-map and -ffile-prefix-map

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn added a comment.

Changes still look reasonable, but the preceding path 
(https://reviews.llvm.org/D56769) needs some work.




Comment at: lib/CodeGen/CGDebugInfo.cpp:607
   llvm::DIFile *CUFile = DBuilder.createFile(
-  remapDIPath(MainFileName), remapDIPath(getCurrentDirname()), CSInfo,
+  MainFileName, remapDIPath(getCurrentDirname()), CSInfo,
   getSource(SM, SM.getMainFileID()));

Any reason for dropping `remapDIPath` here? Wouldn't this result in the full 
path being included even when using:

clang -fdebug-prefix-map=/full/path/= /full/path/source.c



Comment at: lib/Lex/PPMacroExpansion.cpp:1466
+  for (const auto  : MacroPrefixMap)
+if(llvm::sys::path::replace_path_prefix(Path, Entry.first, Entry.second))
+  break;

Style: space between `if` and `(`


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D49466/new/

https://reviews.llvm.org/D49466



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47819: [compiler-rt] [test] Disable sunrpc tests when rpc/xdr.h is missing

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn accepted this revision.
Lekensteyn added a comment.
This revision is now accepted and ready to land.

In D47819#1356498 , @mgorny wrote:

>   PASS: MemorySanitizer-X86_64 :: Linux/sunrpc_bytes.cc (2932 of 6195)
>   PASS: MemorySanitizer-X86_64 :: Linux/sunrpc_string.cc (2935 of 6195)
>   PASS: MemorySanitizer-X86_64 :: Linux/sunrpc.cc (2974 of 6195)
>   PASS: ThreadSanitizer-x86_64 :: sunrpc.cc (5110 of 6195)
>
>
> So yep, it works. Tested both ways now.


Thanks for testing! It looks good to me.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D47819/new/

https://reviews.llvm.org/D47819



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47819: [compiler-rt] [test] Disable sunrpc tests when rpc/xdr.h is missing

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn added a comment.

The direction of this patch looks reasonable to me. Is it worth mentioning the 
issue (https://github.com/google/sanitizers/issues/974) in the commit message?

What environment have you tested this in, could you verify that the tests are 
indeed skipped on a system without these headers, and passed otherwise?




Comment at: lib/sanitizer_common/CMakeLists.txt:195
+set(SANITIZER_COMMON_DEFINITIONS
+  HAVE_RPC_XDR_H=${HAVE_RPC_XDR_H})
 

The old code defined HAVE_xxx to 0 or 1, but in general wouldn't it be 
preferable to define it to 1 if available, and not define it at all if 
unavailable? This would match most other HAVE_xxx checks.

In order to make the lit config work in this case, you have to use something 
like `pythonize_bool(HAVE_RPC_XDR_H)`.



Comment at: test/lit.common.configured.in:39
 set_default("android", @ANDROID_PYBOOL@)
+set_default("have_rpc_xdr_h", @HAVE_RPC_XDR_H@)
 config.available_features.add('target-is-%s' % config.target_arch)

This becomes 0 or 1 instead of True or False, not sure if that will cause 
issues later.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D47819/new/

https://reviews.llvm.org/D47819



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49466: Initial implementation of -fmacro-prefix-map and -ffile-prefix-map

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn accepted this revision.
Lekensteyn added a comment.

Still fine by me, thanks!

As for the commit message, perhaps reference:
https://bugs.llvm.org/show_bug.cgi?id=38135


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D49466/new/

https://reviews.llvm.org/D49466



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49466: Initial implementation of -fmacro-prefix-map and -ffile-prefix-map

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn accepted this revision.
Lekensteyn added a comment.
This revision is now accepted and ready to land.

Tests pass here, using it on a large CMake project with a 
CMAKE_BUILD_TYPE=Debug and c/cxxflags `-ffile-prefix-map=$builddir= 
-ffile-prefix-map=$srcdir/= -fuse-ld=lld` successfully strips all traces of 
`$builddir` and `$srcdir`.

If you could take care of the previous comment (undo the rename or rename 
debug-prefix.map.c), then I've no further comments.

If @joerg or someone else could give the final review/pass, that would be great 
:)




Comment at: lib/Driver/ToolChains/Clang.cpp:612
+if (Map.find('=') == StringRef::npos)
+  D.Diag(diag::err_drv_invalid_argument_to_fdebug_prefix_map) << Map;
+else

joerg wrote:
> I'd prefer the bailout style here, i.e. `if (...) { D.diag(...); continue }`
Wouldn't using `if (...) { D.diag(...); continue; }` also skip the `A->claim()` 
call? Presumably that could result in spurious errors as well about unused 
arguments?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D49466/new/

https://reviews.llvm.org/D49466



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49466: Initial implementation of -fmacro-prefix-map and -ffile-prefix-map

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn added a comment.

Except one thing, it looks reasonable to me. I'll try to run some tests and 
report back tomorrow.

(Not very familiar with Phabricator either. I still see some comments, 
hopefully the "Collapse" function does something useful here.)




Comment at: test/Driver/prefix-map.S:7
+
+// More tests for this flag in debug-prefix-map.c.

Maybe restore the old file name (debug-prefix-map.S) since this still tests the 
debug prefix functionality? And otherwise this comment needs to be updated.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D49466/new/

https://reviews.llvm.org/D49466



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49466: Initial implementation of -fmacro-prefix-map and -ffile-prefix-map

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn added a comment.

Could you add more tests to check the error message for bad options (missing 
`=`):

  -fdebug-prefix-map=bad
  -fmacro-prefix-map=bad
  -ffile-prefix-map=bad

FWIW, GCC emits two errors for `-ffile-prefix-map=bad`.

Another edge case is `-ffile-prefix-map==foo/`, GCC currently uses this to 
prepend `foo/` to every path. Not sure if that is intentional, but that is the 
current behavior (one which is also replicated by this patch I believe).

Could you also mark review comments that are completed as "done"? It should 
make the diff easier to read (I hope) :)




Comment at: include/clang/Basic/DiagnosticDriverKinds.td:118
   "invalid deployment target for -stdlib=libc++ (requires %0 or later)">;
-def err_drv_invalid_argument_to_fdebug_prefix_map : Error<
-  "invalid argument '%0' to -fdebug-prefix-map">;
+def err_drv_invalid_argument_to_prefix_map : Error<
+  "invalid argument '%1' to -%0">;

Maybe rename `_to_prefix_map` to `_to_option`? (And maybe swap the order of 
parameters so `%0` comes before `%1`?)


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D49466/new/

https://reviews.llvm.org/D49466



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49466: Initial implementation of -fmacro-prefix-map and -ffile-prefix-map

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn added a comment.

It would be nice to have this for Clang 8.0, the branch date is within 5 days :)




Comment at: lib/Driver/ToolChains/Clang.cpp:617
 if (Map.find('=') == StringRef::npos)
-  D.Diag(diag::err_drv_invalid_argument_to_fdebug_prefix_map) << Map;
+  D.Diag(diag::err_drv_invalid_argument_to_prefix_map) << Map << "debug";
 else

For `clang -ffile-prefix-map=foo`, wouldn't this report `invalid argument 'foo' 
to -fdebug-prefix-map`? If so, perhaps some method of `A` or `A->getOption()` 
can be used?



Comment at: lib/Driver/ToolChains/Clang.cpp:630
+if (Map.find('=') == StringRef::npos)
+  D.Diag(diag::err_drv_invalid_argument_to_prefix_map) << Map << "macro";
+else

Same concern here about `-ffile-prefix-map=foo` showing an error message about 
`-fmacro-prefix-map`.



Comment at: test/Preprocessor/file_test.c:5
+// RUN: %clang -E -fmacro-prefix-map=%p/= -c -o - %s | FileCheck %s 
--check-prefix CHECK-REMOVE
+//This is a comment
+

Any reason to keep this comment?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D49466/new/

https://reviews.llvm.org/D49466



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47817: [compiler-rt] [sanitizer_common] Fix using libtirpc on Linux

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn added a comment.

There appears to be other definitions for HAVE_TIRPC_RPC_XDR_H in
lib/sanitizer_common/sanitizer_platform.h
lib/sanitizer_common/sanitizer_platform_limits_freebsd.cc

While at it, perhaps the `#if HAVE_FOO` could be replaced by `#ifdef HAVE_FOO`?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D47817/new/

https://reviews.llvm.org/D47817



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54109: [clang-query] continue querying even if files are skipped

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn abandoned this revision.
Lekensteyn added a comment.

This was addressed by https://reviews.llvm.org/D51183 (without new tests 
though).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54109/new/

https://reviews.llvm.org/D54109



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54109: [clang-query] continue querying even if files are skipped

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn updated this revision to Diff 172747.
Lekensteyn retitled this revision from "[clang-query] continue even if files 
are skipped" to "[clang-query] continue querying even if files are skipped".
Lekensteyn added a comment.

Changes:

- Return 1 (instead of 0) if none of the files could be parsed (and add a test 
for it)
- Propagate any error code (like 2 in case of some missing files) from 
`Tool.buildASTs` instead of returning 0.
- Change test to accomodate the change in behavior/output due to 
https://reviews.llvm.org/D51729 (Thanks to Sam McCall for feedback.)


https://reviews.llvm.org/D54109

Files:
  clang-query/tool/ClangQuery.cpp
  test/clang-query/Inputs/database_template.json
  test/clang-query/database-missing-entry.c


Index: test/clang-query/database-missing-entry.c
===
--- /dev/null
+++ test/clang-query/database-missing-entry.c
@@ -0,0 +1,16 @@
+// RUN: rm -rf %t && mkdir -p %t/src %t/build
+// RUN: sed 's|test_dir|%t|g' %S/Inputs/database_template.json > 
%t/build/compile_commands.json
+// RUN: echo 'int A = AVAL;' > %t/src/a.c
+// RUN: echo 'deliberate parsing error' > %t/src/b.c
+// RUN: not clang-query -p=%t/build -c "m integerLiteral()" %t/src/a.c 
%t/src/b.c %t/src/missing.c 2>&1 | FileCheck %s
+
+// Test that neither parse errors nor missing database entries prevent further 
processing.
+// CHECK: deliberate parsing error
+// CHECK: Error while processing {{.*[/\\]}}missing.c.
+// CHECK-NOT-EXIST: Error while processing {{.*[/\\]}}missing.c.
+// CHECK-NOT-EXIST: unable to handle compilation
+// CHECK: a.c:1:9: note: "root" binds here
+
+// Test that an empty AST due to lack of any source files is bad.
+// RUN: not clang-query -p=%t/build -c "m integerLiteral()" %t/src/missing.c 
2>&1 | FileCheck --check-prefix=CHECK-NONE %s
+// CHECK-NONE: Error while processing {{.*[/\\]}}missing.c.
Index: test/clang-query/Inputs/database_template.json
===
--- /dev/null
+++ test/clang-query/Inputs/database_template.json
@@ -0,0 +1,12 @@
+[
+{
+  "directory": "test_dir/build",
+  "command": "clang -DAVAL=8 -o a.o test_dir/src/a.c",
+  "file": "test_dir/src/a.c"
+},
+{
+  "directory": "test_dir/build",
+  "command": "clang -o b.o test_dir/src/b.c",
+  "file": "test_dir/src/b.c"
+}
+]
Index: clang-query/tool/ClangQuery.cpp
===
--- clang-query/tool/ClangQuery.cpp
+++ clang-query/tool/ClangQuery.cpp
@@ -100,8 +100,9 @@
   ClangTool Tool(OptionsParser.getCompilations(),
  OptionsParser.getSourcePathList());
   std::vector> ASTs;
-  if (Tool.buildASTs(ASTs) != 0)
-return 1;
+  int RunResult = Tool.buildASTs(ASTs);
+  if (ASTs.empty())
+  return 1;
 
   QuerySession QS(ASTs);
 
@@ -134,5 +135,5 @@
 }
   }
 
-  return 0;
+  return RunResult;
 }


Index: test/clang-query/database-missing-entry.c
===
--- /dev/null
+++ test/clang-query/database-missing-entry.c
@@ -0,0 +1,16 @@
+// RUN: rm -rf %t && mkdir -p %t/src %t/build
+// RUN: sed 's|test_dir|%t|g' %S/Inputs/database_template.json > %t/build/compile_commands.json
+// RUN: echo 'int A = AVAL;' > %t/src/a.c
+// RUN: echo 'deliberate parsing error' > %t/src/b.c
+// RUN: not clang-query -p=%t/build -c "m integerLiteral()" %t/src/a.c %t/src/b.c %t/src/missing.c 2>&1 | FileCheck %s
+
+// Test that neither parse errors nor missing database entries prevent further processing.
+// CHECK: deliberate parsing error
+// CHECK: Error while processing {{.*[/\\]}}missing.c.
+// CHECK-NOT-EXIST: Error while processing {{.*[/\\]}}missing.c.
+// CHECK-NOT-EXIST: unable to handle compilation
+// CHECK: a.c:1:9: note: "root" binds here
+
+// Test that an empty AST due to lack of any source files is bad.
+// RUN: not clang-query -p=%t/build -c "m integerLiteral()" %t/src/missing.c 2>&1 | FileCheck --check-prefix=CHECK-NONE %s
+// CHECK-NONE: Error while processing {{.*[/\\]}}missing.c.
Index: test/clang-query/Inputs/database_template.json
===
--- /dev/null
+++ test/clang-query/Inputs/database_template.json
@@ -0,0 +1,12 @@
+[
+{
+  "directory": "test_dir/build",
+  "command": "clang -DAVAL=8 -o a.o test_dir/src/a.c",
+  "file": "test_dir/src/a.c"
+},
+{
+  "directory": "test_dir/build",
+  "command": "clang -o b.o test_dir/src/b.c",
+  "file": "test_dir/src/b.c"
+}
+]
Index: clang-query/tool/ClangQuery.cpp
===
--- clang-query/tool/ClangQuery.cpp
+++ clang-query/tool/ClangQuery.cpp
@@ -100,8 +100,9 @@
   ClangTool Tool(OptionsParser.getCompilations(),
  OptionsParser.getSourcePathList());
   std::vector> ASTs;
-  if (Tool.buildASTs(ASTs) != 0)
-return 1;
+  int RunResult = Tool.buildASTs(ASTs);
+  if (ASTs.empty())
+  return 1;
 
   

[PATCH] D51729: [Tooling] JSONCompilationDatabasePlugin infers compile commands for missing files

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn added a comment.

In https://reviews.llvm.org/D51729#1288360, @sammccall wrote:

> > I'm not entirely sure what to do here. The old behavior works great in 
> > cases where a complete database is available (produced by CMake). The new 
> > behavior might work better for clangd (?), but it breaks a use case (see 
> > above).
>
> For clangd, but also clang-tidy and clang-query when the user *does* want to 
> use it on files not represented in the CDB. (e.g. stale or headers)
>  There's indeed a tension here, because the CDB discovery needs to have a 
> default configuration.


Support for querying header files seems valuable and a good argument to keep 
this change.

> That said, in this case the behavior looks appropriate to me: you've 
> explicitly specified files on the command line, ignoring them and returning 
> with status 0 seems surprising.

The test explicitly lists them, but in practice the `find` or `grep -rl` 
provides the file names which might contain uninteresting matches that are not 
present in the CDB. As a compromise, it could return with status 1 but proceed 
querying files that were successfully parsed.

> For the case of "search over all TUs in CDB", the CDB does offer the ability 
> to list TUs and iterate over compile commands, and ClangTool lets you run in 
> this mode. We've discussed in the past adding a filename filter to 
> `AllTUsExecutor`, which would be useful for this purpose and others. @ioeric

`AllTUsToolExecutor` looks useful to enable concurrency, but a filename filter 
would not help in my case. The reason I do a "cheap" grep first before using 
clang-query is because building the AST is slow and consumes a lot of memory 
(after ~1k .c files, 12G was in use).

Querying the CDB and filtering out entries is currently possible, but quite 
verbose:

  grep -le 'search term' $(jq -r '.[].file' compile_commands.json) | xargs 
clang-query -c 'm ...'

Thank you for the feedback, my concerns with this CDB patch has been resolved.


Repository:
  rC Clang

https://reviews.llvm.org/D51729



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51729: [Tooling] JSONCompilationDatabasePlugin infers compile commands for missing files

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn added a comment.

Before this patch, missing compilation database entries resulted in "Skipping 
 Compile command not found." which is assumed by the tests in this 
clang-query patch: https://reviews.llvm.org/D54109

After this patch, a command is inferred, but this is not always desired.
Use case: find all function calls with a certain name using the compilation 
database produced by CMake.
Example command:

  clang-query -p=/tmp/wsbuild -c 'set output print' -c 'm 
callExpr(callee(functionDecl(hasName("uat_new"' $(grep -rl uat_new 
epan/dissectors/)

Expected result for some template files which do not exist in the compilation 
database:

  Skipping .../epan/dissectors/asn1/x509af/packet-x509af-template.c. Compile 
command not found.

Actual result (clang-query tries to parse the contents which will fail since it 
is not an actual C source file):

  .../epan/dissectors/asn1/x509af/packet-x509af-template.c:18:10: fatal error: 
'packet-ber.h' file not found

I'm not entirely sure what to do here. The old behavior works great in cases 
where a complete database is available (produced by CMake). The new behavior 
might work better for clangd (?), but it breaks a use case (see above).


Repository:
  rC Clang

https://reviews.llvm.org/D51729



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54109: [clang-query] continue even if files are skipped

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn added inline comments.



Comment at: test/clang-query/database-missing-entry.c:9
+// CHECK: deliberate parsing error
+// CHECK: Skipping {{.*[/\\]}}missing.c. Compile command not found.
+// CHECK-NOT-EXIST: Error while processing {{.*[/\\]}}missing.c.

On trunk this fails tests because of https://reviews.llvm.org/D51729. Reverting 
that patch will properly ignore the missing file.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54109



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54109: [clang-query] continue even if files are skipped

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn created this revision.
Lekensteyn added reviewers: cfe-commits, pcc.

Like clang-check, continue even if some source files were not found in
the compilation database. This makes it possible to do search for
matching sources and query files that exist in the compilation db:

  clang-query -c 'm callExpr(callee(functionDecl(hasName("function_name"' 
$(grep -rl ^function_name)

Note: buildASTs calls ClangTool::run which returns 1 if any of the
commands failed and 2 otherwise if any of the files were missing.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54109

Files:
  clang-query/tool/ClangQuery.cpp
  test/clang-query/Inputs/database_template.json
  test/clang-query/database-missing-entry.c


Index: test/clang-query/database-missing-entry.c
===
--- /dev/null
+++ test/clang-query/database-missing-entry.c
@@ -0,0 +1,12 @@
+// RUN: rm -rf %t && mkdir -p %t/src %t/build
+// RUN: sed 's|test_dir|%t|g' %S/Inputs/database_template.json > 
%t/build/compile_commands.json
+// RUN: echo 'int A = AVAL;' > %t/src/a.c
+// RUN: echo 'deliberate parsing error' > %t/src/b.c
+// RUN: clang-query -p=%t/build -c "m integerLiteral()" %t/src/a.c %t/src/b.c 
%t/src/missing.c 2>&1 | FileCheck %s
+
+// Test that neither parse errors nor missing database entries prevent further 
processing.
+// CHECK: deliberate parsing error
+// CHECK: Skipping {{.*[/\\]}}missing.c. Compile command not found.
+// CHECK-NOT-EXIST: Error while processing {{.*[/\\]}}missing.c.
+// CHECK-NOT-EXIST: unable to handle compilation
+// CHECK: a.c:1:9: note: "root" binds here
Index: test/clang-query/Inputs/database_template.json
===
--- /dev/null
+++ test/clang-query/Inputs/database_template.json
@@ -0,0 +1,12 @@
+[
+{
+  "directory": "test_dir/build",
+  "command": "clang -DAVAL=8 -o a.o test_dir/src/a.c",
+  "file": "test_dir/src/a.c"
+},
+{
+  "directory": "test_dir/build",
+  "command": "clang -o b.o test_dir/src/b.c",
+  "file": "test_dir/src/b.c"
+}
+]
Index: clang-query/tool/ClangQuery.cpp
===
--- clang-query/tool/ClangQuery.cpp
+++ clang-query/tool/ClangQuery.cpp
@@ -100,8 +100,7 @@
   ClangTool Tool(OptionsParser.getCompilations(),
  OptionsParser.getSourcePathList());
   std::vector> ASTs;
-  if (Tool.buildASTs(ASTs) != 0)
-return 1;
+  Tool.buildASTs(ASTs);
 
   QuerySession QS(ASTs);
 


Index: test/clang-query/database-missing-entry.c
===
--- /dev/null
+++ test/clang-query/database-missing-entry.c
@@ -0,0 +1,12 @@
+// RUN: rm -rf %t && mkdir -p %t/src %t/build
+// RUN: sed 's|test_dir|%t|g' %S/Inputs/database_template.json > %t/build/compile_commands.json
+// RUN: echo 'int A = AVAL;' > %t/src/a.c
+// RUN: echo 'deliberate parsing error' > %t/src/b.c
+// RUN: clang-query -p=%t/build -c "m integerLiteral()" %t/src/a.c %t/src/b.c %t/src/missing.c 2>&1 | FileCheck %s
+
+// Test that neither parse errors nor missing database entries prevent further processing.
+// CHECK: deliberate parsing error
+// CHECK: Skipping {{.*[/\\]}}missing.c. Compile command not found.
+// CHECK-NOT-EXIST: Error while processing {{.*[/\\]}}missing.c.
+// CHECK-NOT-EXIST: unable to handle compilation
+// CHECK: a.c:1:9: note: "root" binds here
Index: test/clang-query/Inputs/database_template.json
===
--- /dev/null
+++ test/clang-query/Inputs/database_template.json
@@ -0,0 +1,12 @@
+[
+{
+  "directory": "test_dir/build",
+  "command": "clang -DAVAL=8 -o a.o test_dir/src/a.c",
+  "file": "test_dir/src/a.c"
+},
+{
+  "directory": "test_dir/build",
+  "command": "clang -o b.o test_dir/src/b.c",
+  "file": "test_dir/src/b.c"
+}
+]
Index: clang-query/tool/ClangQuery.cpp
===
--- clang-query/tool/ClangQuery.cpp
+++ clang-query/tool/ClangQuery.cpp
@@ -100,8 +100,7 @@
   ClangTool Tool(OptionsParser.getCompilations(),
  OptionsParser.getSourcePathList());
   std::vector> ASTs;
-  if (Tool.buildASTs(ASTs) != 0)
-return 1;
+  Tool.buildASTs(ASTs);
 
   QuerySession QS(ASTs);
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49466: Initial implementation of -fmacro-prefix-map and -ffile-prefix-map

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn added inline comments.



Comment at: lib/Lex/PPMacroExpansion.cpp:1460
+  for (const auto  : MacroPrefixMap)
+if (Path.startswith(Entry.first))
+  return (Twine(Entry.second) + Path.substr(Entry.first.size())).str();

joerg wrote:
> Lekensteyn wrote:
> > dankm wrote:
> > > dankm wrote:
> > > > joerg wrote:
> > > > > This doesn't handle directory vs string prefix prefix correctly, does 
> > > > > it? At the very least, it should have a test case :)
> > > > Good catch. I expect not. But on the other hand, it's exactly what 
> > > > debug-prefix-map does :)
> > > > 
> > > > I'll add test cases in a future review. My first goal was getting 
> > > > something sort-of working.
> > > There should be a test, but apparently the debug prefix map code also 
> > > does this.
> > > 
> > > What do you think the correct behaviour should be? a string prefix, or a 
> > > directory prefix?
> > It should be a string prefix (like GCC)
> I disagree. I consider it a bug in GCC that it is a string prefix. It's quite 
> inconsistent as well.
I agree with you, it should have been a directory prefix but GCC implements it 
as a string prefix although the GCC documents it as:
"-fdebug-prefix-map=old=new When compiling files residing in **directory old**, 
record debugging information describing them as if the files resided in 
**directory new** instead."

If you decide to fix `-fmacro-prefix-map` to use a directory prefix match, then 
the `-fdebug-prefix-map` should also be fixed for consistency. What about 
implementing the (buggy) GCC-compatible behavior first and then fixing both 
cases in a future patch? (I don't mind when the buggy behavior is fixed, I just 
want to see this functionality moving forward.)

Another edge case that I ran into is when using the option to drop directories. 
When using `-ffile-prefix-map=/src=`, the command `cd /src && cc /src/foo.c` 
would have `__FILE__` equal to `/foo.c`. As a native "fix", one would try 
`-ffile-prefix-map=/src/=` which indeed produces `__FILE__` equal to `foo.c`.

Matching with a trailing slash however fails to correctly remap some debug 
information, namely `DW_AT_comp_dir`. This contains the working directory 
(`/src`) which is not matched by `/src/`. By using a proper directory prefix 
match, this would be nicely fixed.


Repository:
  rC Clang

https://reviews.llvm.org/D49466



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49466: Initial implementation of -fmacro-prefix-map and -ffile-prefix-map

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn requested changes to this revision.
Lekensteyn added a comment.
This revision now requires changes to proceed.

The functionality looks correct to me, but could you include some tests in 
test/Driver/ and test/Preprocessor/ just to be sure?
test/Driver/debug-prefix-map.c and test/CodeGen/debug-prefix-map.c could serve 
as inspiration.

The documentation should probable be updated too: 
docs/ClangCommandLineReference.rst

(It would be nice to have this feature for Reproducible Builds)




Comment at: lib/Lex/PPMacroExpansion.cpp:1460
+  for (const auto  : MacroPrefixMap)
+if (Path.startswith(Entry.first))
+  return (Twine(Entry.second) + Path.substr(Entry.first.size())).str();

dankm wrote:
> dankm wrote:
> > joerg wrote:
> > > This doesn't handle directory vs string prefix prefix correctly, does it? 
> > > At the very least, it should have a test case :)
> > Good catch. I expect not. But on the other hand, it's exactly what 
> > debug-prefix-map does :)
> > 
> > I'll add test cases in a future review. My first goal was getting something 
> > sort-of working.
> There should be a test, but apparently the debug prefix map code also does 
> this.
> 
> What do you think the correct behaviour should be? a string prefix, or a 
> directory prefix?
It should be a string prefix (like GCC)


Repository:
  rC Clang

https://reviews.llvm.org/D49466



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47817: [sanitizer_common] Fix using libtirpc on Linux

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn added a comment.

In https://reviews.llvm.org/D47817#1155717, @mgorny wrote:

> > This would be the first user of pkg-config here. I am not sure if this 
> > would be the best fix. Usually you cannot (easily) recompile libc and 
> > override it, but for external libs such as libtirpc this should be more 
> > doable (I think).
>
> I don't think libtirpc's include path is expected to be predictable by 
> design. I think it's something distro maintainers have to choose to avoid 
> collision with headers that (used to be) installed by glibc. In any case, I 
> can't think of a better solution than pkg-config here (libtirpc doesn't come 
> with CMake modules).


On Arch (libtirpc-1.0.3-2), Debian (libtirpc-dev 0.2.5-1.2 in sid), Gentoo 
(libtirpc-1.0.3), the include files happen to be installed in 
/usr/include/libtirpc, so it seems pretty consistent. So you could consider 
writing a `cmake/Modules/FindLibtirpc.cmake` module that looks like:

  # sets Libtirpc_FOUND Libtirpc_INCLUDE_DIRS
  
  find_path(Libtirpc_INCLUDE_DIR
NAMES rpc/xdr.h
PATH_SUFFIXES tirpc
  )
  include(FindPackageHandleStandardArgs)
  find_package_handle_standard_args(Libtirpc REQUIRED_VARS Libtirpc_INCLUDE_DIR)
  if(Libtirpc_FOUND)
set(Libtirpc_INCLUDE_DIRS ${Libtirpc_INCLUDE_DIR})
  endif()

then you can use `find_package(Libtirpc)` without depending on `pkg-config`.

> libtirpc case never could have worked.

The reason for that is probably because of 
lib/sanitizer_common/sanitizer_platform.h saying:

  // Assume obsolete RPC headers are available by default
  #if !defined(HAVE_RPC_XDR_H) && !defined(HAVE_TIRPC_RPC_XDR_H)
  # define HAVE_RPC_XDR_H (SANITIZER_LINUX && !SANITIZER_ANDROID)
  # define HAVE_TIRPC_RPC_XDR_H 0
  #endif

That should probably be addressed too.


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D47817



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47817: [sanitizer_common] Fix using libtirpc on Linux

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn added a comment.

Hi, thank you for the patch. First a disclaimer, I am not familiar with this 
RPC API at all.

This would be the first user of pkg-config here. I am not sure if this would be 
the best fix. Usually you cannot (easily) recompile libc and override it, but 
for external libs such as libtirpc this should be more doable (I think).

I'm not comfortable with adding the tirpc include path to the default include 
path and stripping `-nodefaultlibs` either, would this approach work for 
cross-compilation?


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D47817



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46522: [clang] cmake: resolve symlinks in ClangConfig.cmake

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn added a comment.

In https://reviews.llvm.org/D46522#1096847, @kimgr wrote:

> I'm interested in this, I've tried for a while to fix the Debian packaging 
> but I'm completely new to the packaging toolchain, so I'm making slow headway.


The Debian clang-5.0 1:5.0.2-2 package already includes this patch. I tried to 
upstream it, but there were some concerns about the real path not always being 
the desired value.

> My (possibly naive) take is that since the LLVM/Clang build/install tree 
> works as-is with `find_package`, the bug must be in packaging. That is, if 
> you have a local build tree in `/build/`, this configures without a hitch: 
> `cmake -DCMAKE_PREFIX_PATH=/build/ -G Ninja .` with a simple `CMakeLists.txt` 
> doing `find_package` for both LLVM and Clang.

On Debian, `cmake -DCMAKE_PREFIX_PATH=/usr/lib/llvm-5.0` would work as well, 
the problem was that the FindClang.cmake file was installed into an unexpected 
location (packaging issue).

Secondary to that, some additional symlinks were installed to ensure that users 
do not have to set `CMAKE_PREFIX_PATH` in order to find some LLVM/Clang 
version. That scenario was being addressed with this patch (and 
https://reviews.llvm.org/D46521).


Repository:
  rC Clang

https://reviews.llvm.org/D46522



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46522: [clang] cmake: resolve symlinks in ClangConfig.cmake

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn abandoned this revision.
Lekensteyn added a comment.

Per discussion in https://reviews.llvm.org/D46521, resolving symlinks might 
introduce issues that are are worse than the issue that was solved here. It 
seems better to carry this patch downstream (in Debian) since the problem only 
exists there due to its use of symlinks (which is apparently not fully 
supported/recommended in LLVM upstream).


Repository:
  rC Clang

https://reviews.llvm.org/D46522



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46522: [clang] cmake: resolve symlinks in ClangConfig.cmake

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn added reviewers: brad.king, gottesmm, mgorny.
Lekensteyn added a comment.

Hi, this patch was required to make `find_package(Clang CONFIG)` work when 
`/usr/lib/llvm/clang-X.Y` was a symlink and is integrated in the Debian 
packaging. Please have a look, thanks!

One unfortunate limitation (that existed before) is that `find_package(Clang 
5.0)` does not work because `CLANG_VERSION_*` is not defined. So
`find_package(LLVM 6.0)` could locate 
`/usr/lib/cmake/llvm-6.0/LLVMConfig.cmake` but then
`find_package(Clang)` could end up using `/usr/lib/cmake/clang-5.0`. (This was 
discovered while testing a Clang 6.0 build on a system with Clang 5.0 
co-installed.)

The reason for me to use different Clang versions is for CI purposes. As such, 
I am currently using `CMAKE_PREFIX_PATH=/usr/lib/llvm-6.0` which will find the 
correct version without explicitly having to specify it in `find_package`.


Repository:
  rC Clang

https://reviews.llvm.org/D46522



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46522: [clang] cmake: resolve symlinks in ClangConfig.cmake

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn created this revision.
Lekensteyn added a reviewer: sylvestre.ledru.
Herald added subscribers: cfe-commits, mgorny.

Ensure that symlinks such as /usr/lib/cmake/clang-X.Y (pointing to
/usr/lib/llvm-X.Y/lib/cmake/llvm) are resolved. This ensures that
CLANG_INSTALL_PREFIX ends up to be /usr/lib/llvm-X.Y instead of /usr.

Partially addresses PR37128


Repository:
  rC Clang

https://reviews.llvm.org/D46522

Files:
  cmake/modules/CMakeLists.txt


Index: cmake/modules/CMakeLists.txt
===
--- cmake/modules/CMakeLists.txt
+++ cmake/modules/CMakeLists.txt
@@ -30,10 +30,10 @@
 # Generate ClangConfig.cmake for the install tree.
 set(CLANG_CONFIG_CODE "
 # Compute the installation prefix from this LLVMConfig.cmake file location.
-get_filename_component(CLANG_INSTALL_PREFIX \"\${CMAKE_CURRENT_LIST_FILE}\" 
PATH)")
+get_filename_component(CLANG_INSTALL_PREFIX \"\${CMAKE_CURRENT_LIST_FILE}\" 
REALPATH)")
 # Construct the proper number of get_filename_component(... PATH)
 # calls to compute the installation prefix.
-string(REGEX REPLACE "/" ";" _count "${CLANG_INSTALL_PACKAGE_DIR}")
+string(REGEX REPLACE "/" ";" _count "prefix/${CLANG_INSTALL_PACKAGE_DIR}")
 foreach(p ${_count})
   set(CLANG_CONFIG_CODE "${CLANG_CONFIG_CODE}
 get_filename_component(CLANG_INSTALL_PREFIX \"\${CLANG_INSTALL_PREFIX}\" 
PATH)")


Index: cmake/modules/CMakeLists.txt
===
--- cmake/modules/CMakeLists.txt
+++ cmake/modules/CMakeLists.txt
@@ -30,10 +30,10 @@
 # Generate ClangConfig.cmake for the install tree.
 set(CLANG_CONFIG_CODE "
 # Compute the installation prefix from this LLVMConfig.cmake file location.
-get_filename_component(CLANG_INSTALL_PREFIX \"\${CMAKE_CURRENT_LIST_FILE}\" PATH)")
+get_filename_component(CLANG_INSTALL_PREFIX \"\${CMAKE_CURRENT_LIST_FILE}\" REALPATH)")
 # Construct the proper number of get_filename_component(... PATH)
 # calls to compute the installation prefix.
-string(REGEX REPLACE "/" ";" _count "${CLANG_INSTALL_PACKAGE_DIR}")
+string(REGEX REPLACE "/" ";" _count "prefix/${CLANG_INSTALL_PACKAGE_DIR}")
 foreach(p ${_count})
   set(CLANG_CONFIG_CODE "${CLANG_CONFIG_CODE}
 get_filename_component(CLANG_INSTALL_PREFIX \"\${CLANG_INSTALL_PREFIX}\" PATH)")
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37954: Try to shorten system header paths when using -MD depfiles

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn added a comment.

(FWIW, the suggestion to use `FileManager:getCanonicalName` did not work 
because I have no DirectoryEntry. And `-no-canonical-prefixes` it used very 
early and stripped from normal options, it seems used for a different purpose. 
Given the opposition, I think that this patch is a no-go.)

In https://reviews.llvm.org/D37954#902979, @jyknight wrote:

> It seems to me that it was caused by the prefix being set as "/bin" instead 
> of "/usr/bin", because clang _doesn't_ actually canonicalize its prefix, even 
> when -no-canonical-prefixes isn't specified! All it does, now, is to make the 
> prefix absolute -- without fully canonicalizing. I think that's simply a bug.
>
> If the prefix had been, properly, /usr/bin, then the include path would've 
> been:
>
>   
> /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/7.2.0/../../../../include/c++/7.2.0/iostream
>
> And in that case, it would've worked properly with ninja, without adding the 
> feature in this patch.
>
> Reference clang/tools/driver/driver.cpp:297:
>
>   // FIXME: We don't actually canonicalize this, we just make it absolute.
>   if (CanonicalPrefixes)
> llvm::sys::fs::make_absolute(InstalledPath);


If this fixes the issue, then it would definitely be a much better approach. If 
this has changed between Clang 4 -> 5, then it is possible the root cause.

On my system (Arch Linux) `/bin` is a symlink to `usr/bin`. Output of `clang 
-v`:

  clang version 5.0.0 (tags/RELEASE_500/final)
  Target: x86_64-unknown-linux-gnu
  Thread model: posix
  InstalledDir: /bin
  Found candidate GCC installation: /bin/../lib/gcc/x86_64-pc-linux-gnu/7.2.0
  Found candidate GCC installation: /bin/../lib64/gcc/x86_64-pc-linux-gnu/7.2.0
  Found candidate GCC installation: /usr/lib/gcc/x86_64-pc-linux-gnu/7.2.0
  Found candidate GCC installation: /usr/lib64/gcc/x86_64-pc-linux-gnu/7.2.0
  Selected GCC installation: /bin/../lib64/gcc/x86_64-pc-linux-gnu/7.2.0
  Candidate multilib: .;@m64
  Candidate multilib: 32;@m32
  Selected multilib: .;@m64


Repository:
  rL LLVM

https://reviews.llvm.org/D37954



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37954: Try to shorten system header paths when using -MD depfiles

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn added a comment.

In https://reviews.llvm.org/D37954#901878, @rsmith wrote:

> In https://reviews.llvm.org/D37954#884029, @Lekensteyn wrote:
>
> > Any objection for merging these patches? I rebased today (no changes 
> > needed) and it still passes clang-tests.
>
>
> This is not the way that we do code review in LLVM and Clang. If you don't 
> get a response to a patch, you're expected to keep pinging it (typically once 
> a week) until you do get an answer.


There was no negative feedback and looking in the git logs also showed some 
examples without explicit review approval, so I (mistakenly) thought it was 
acceptable.

>   This past week in particular has been especially bad as we had the mailing 
> deadline for the C++ committee meeting on Monday and the LLVM developer 
> meeting yesterday and today.

Oh, where could I have learned about this? I am sporadically contributing and 
not following every day.

> I reverted this in r316195, because it breaks users using 
> `-fno-canonical-prefixes` to request that we do not do this kind of path 
> canonicalization.

I can propose a new patch and also add a test for this case. Thanks for your 
review and pointers!

In https://reviews.llvm.org/D37954#901884, @joerg wrote:

> The behavior of sometimes transforming the path and sometimes not is IMO 
> completely unacceptable. I don't care if GCC created that bug first.


Would it help if a compile-time option is added so you can disable that for 
OpenBSD? All I am trying to do is to fix a bug in Ninja that is triggered 
because Clang deviates from GCC here. And Ninja will apparently not be fixed, 
so that leaves only this option. Do you have an alternative proposal?




Comment at: cfe/trunk/lib/Driver/ToolChains/Clang.cpp:967-968
 
+  if (Arg *A = Args.getLastArg(options::OPT_fno_canonical_system_headers,
+   options::OPT_fcanonical_system_headers)) {
+if (A->getOption().matches(options::OPT_fno_canonical_system_headers)) {

rsmith wrote:
> Use `hasFlag` instead.
> 
> Also, defaulting this on will break existing users who use 
> `-fno-canonical-prefixes` to turn off the (wrong in general) assumption that 
> using `realpath` is meaningful. Please default to the value of that flag 
> instead. I would guess (but haven't checked) that flag controls this behavior 
> in GCC.
Ok, I will apply the suggestions.

This patch does not seem to have any effect on clang, the path still starts 
with `/bin/../lib64/...` rather than `/usr/lib/...`

With GCC: just `-no-canonical-prefixes` does not have any effect.

`-fno-canonical-system-headers -no-canonical-prefixes` produces:
`/bin/../lib/gcc/x86_64-pc-linux-gnu/7.2.0/../../../../include/c++/7.2.0/cstdio`

`-fno-canonical-system-headers` produces:
`/usr/lib/gcc/x86_64-pc-linux-gnu/7.2.0/../../../../include/c++/7.2.0/cstdio`



Comment at: cfe/trunk/lib/Frontend/DependencyFile.cpp:296
+  if (CanonicalSystemHeaders && isSystem(FileType)) {
+StringRef RealPath = FE->tryGetRealPathName();
+if (!RealPath.empty() && RealPath.size() < Filename.size()) {

rsmith wrote:
> This is not an appropriate use of `tryGetRealPathName`. Its purpose is to get 
> the way the file name was stored in disk (particularly, preserving the 
> original case on a case-sensitive file system), not to resolve symlinks.
> 
> `FileManager::getCanonicalName` would be the right thing to call.
OK.


Repository:
  rL LLVM

https://reviews.llvm.org/D37954



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37954: Try to shorten system header paths when using -MD depfiles

[Peter Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL316193: Try to shorten system header paths when using -MD 
depfiles (authored by Lekensteyn).

Changed prior to commit:
  https://reviews.llvm.org/D37954?vs=115946=119626#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D37954

Files:
  cfe/trunk/include/clang/Driver/Options.td
  cfe/trunk/include/clang/Frontend/DependencyOutputOptions.h
  cfe/trunk/lib/Driver/Job.cpp
  cfe/trunk/lib/Driver/ToolChains/Clang.cpp
  cfe/trunk/lib/Frontend/CompilerInvocation.cpp
  cfe/trunk/lib/Frontend/DependencyFile.cpp
  cfe/trunk/lib/Tooling/ArgumentsAdjusters.cpp
  cfe/trunk/test/Preprocessor/dependencies-realpath.c

Index: cfe/trunk/test/Preprocessor/dependencies-realpath.c
===
--- cfe/trunk/test/Preprocessor/dependencies-realpath.c
+++ cfe/trunk/test/Preprocessor/dependencies-realpath.c
@@ -0,0 +1,33 @@
+// RUN: mkdir -p %t/sub/dir
+// RUN: echo > %t/sub/empty.h
+
+// Test that system header paths are expanded
+//
+// RUN: %clang -fsyntax-only -MD -MF %t.d -MT foo %s -isystem %t/sub/dir/..
+// RUN: FileCheck -check-prefix=TEST1 %s < %t.d
+// TEST1: foo:
+// TEST1: sub{{/|\\}}empty.h
+
+// Test that system header paths are not expanded to a longer form
+//
+// RUN: cd %t && %clang -fsyntax-only -MD -MF %t.d -MT foo %s -isystem sub/dir/..
+// RUN: FileCheck -check-prefix=TEST2 %s < %t.d
+// TEST2: foo:
+// TEST2: sub/dir/..{{/|\\}}empty.h
+
+// Test that user header paths are not expanded
+//
+// RUN: %clang -fsyntax-only -MD -MF %t.d -MT foo %s -I %t/sub/dir/..
+// RUN: FileCheck -check-prefix=TEST3 %s < %t.d
+// TEST3: foo:
+// TEST3: sub/dir/..{{/|\\}}empty.h
+
+// Test that system header paths are not expanded with -fno-canonical-system-headers
+// (and also that the -fsystem-system-headers option is accepted)
+//
+// RUN: %clang -fsyntax-only -MD -MF %t.d -MT foo %s -I %t/sub/dir/.. -fcanonical-system-headers -fno-canonical-system-headers
+// RUN: FileCheck -check-prefix=TEST4 %s < %t.d
+// TEST4: foo:
+// TEST4: sub/dir/..{{/|\\}}empty.h
+
+#include 
Index: cfe/trunk/lib/Driver/Job.cpp
===
--- cfe/trunk/lib/Driver/Job.cpp
+++ cfe/trunk/lib/Driver/Job.cpp
@@ -73,8 +73,8 @@
 
   // These flags are all of the form -Flag and have no second argument.
   ShouldSkip = llvm::StringSwitch(Flag)
-.Cases("-M", "-MM", "-MG", "-MP", "-MD", true)
-.Case("-MMD", true)
+.Cases("-M", "-MM", "-MG", "-MP", "-MD", "-MMD", true)
+.Cases("-fno-canonical-system-headers", "-fcanonical-system-headers", true)
 .Default(false);
 
   // Match found.
Index: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp
@@ -964,6 +964,13 @@
   Args.AddLastArg(CmdArgs, options::OPT_C);
   Args.AddLastArg(CmdArgs, options::OPT_CC);
 
+  if (Arg *A = Args.getLastArg(options::OPT_fno_canonical_system_headers,
+   options::OPT_fcanonical_system_headers)) {
+if (A->getOption().matches(options::OPT_fno_canonical_system_headers)) {
+  CmdArgs.push_back("-fno-canonical-system-headers");
+}
+  }
+
   // Handle dependency file generation.
   if ((A = Args.getLastArg(options::OPT_M, options::OPT_MM)) ||
   (A = Args.getLastArg(options::OPT_MD)) ||
Index: cfe/trunk/lib/Frontend/CompilerInvocation.cpp
===
--- cfe/trunk/lib/Frontend/CompilerInvocation.cpp
+++ cfe/trunk/lib/Frontend/CompilerInvocation.cpp
@@ -1002,6 +1002,7 @@
   Opts.Targets = Args.getAllArgValues(OPT_MT);
   Opts.IncludeSystemHeaders = Args.hasArg(OPT_sys_header_deps);
   Opts.IncludeModuleFiles = Args.hasArg(OPT_module_file_deps);
+  Opts.CanonicalSystemHeaders = !Args.hasArg(OPT_fno_canonical_system_headers);
   Opts.UsePhonyTargets = Args.hasArg(OPT_MP);
   Opts.ShowHeaderIncludes = Args.hasArg(OPT_H);
   Opts.HeaderIncludeOutputFile = Args.getLastArgValue(OPT_header_include_file);
Index: cfe/trunk/lib/Frontend/DependencyFile.cpp
===
--- cfe/trunk/lib/Frontend/DependencyFile.cpp
+++ cfe/trunk/lib/Frontend/DependencyFile.cpp
@@ -161,6 +161,7 @@
   bool AddMissingHeaderDeps;
   bool SeenMissingHeader;
   bool IncludeModuleFiles;
+  bool CanonicalSystemHeaders;
   DependencyOutputFormat OutputFormat;
 
 private:
@@ -176,6 +177,7 @@
   AddMissingHeaderDeps(Opts.AddMissingHeaderDeps),
   SeenMissingHeader(false),
   IncludeModuleFiles(Opts.IncludeModuleFiles),
+  CanonicalSystemHeaders(Opts.CanonicalSystemHeaders),
   OutputFormat(Opts.OutputFormat) {
 for (const auto  : Opts.ExtraDeps) {
   AddFilename(ExtraDep);
@@ -288,6 +290,15 @@
   if (!FileMatchesDepCriteria(Filename.data(), FileType))
 return;
 
+  // Try to 

[PATCH] D37954: Try to shorten system header paths when using -MD depfiles

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn added a comment.

ping? I'll push it next week if there is no new feedback.


https://reviews.llvm.org/D37954



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37954: Try to shorten system header paths when using -MD depfiles

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn added a comment.

Any objection for merging these patches? I rebased today (no changes needed) 
and it still passes clang-tests.


https://reviews.llvm.org/D37954



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37954: Try to shorten system header paths when using -MD depfiles

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn updated this revision to Diff 115946.
Lekensteyn retitled this revision from "Expand absolute system header paths 
when using -MD depfiles" to "Try to shorten system header paths when using -MD 
depfiles".
Lekensteyn edited the summary of this revision.
Lekensteyn added a comment.

Thanks for the feedback, I have the patch accordingly. Changes in v2:

- Add (no-op) -fcanonical-system-headers option as suggested by @thakis (yes, 
GCC supports this option, but GCC also has a configure option that can change 
the default which Clang does not have)
- Check whether path is shorter instead of checking for absolute path (matches 
the original GCC bug, should address @joerg's concern)
- Update tests to use "%t" instead of "%T" (noticed 
https://reviews.llvm.org/D35396 after looking for whether "mkdir -p" is 
allowed) + other minor text/redundancy tweaks


https://reviews.llvm.org/D37954

Files:
  include/clang/Driver/Options.td
  include/clang/Frontend/DependencyOutputOptions.h
  lib/Driver/Job.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/Frontend/DependencyFile.cpp
  lib/Tooling/ArgumentsAdjusters.cpp
  test/Preprocessor/dependencies-realpath.c

Index: test/Preprocessor/dependencies-realpath.c
===
--- /dev/null
+++ test/Preprocessor/dependencies-realpath.c
@@ -0,0 +1,33 @@
+// RUN: mkdir -p %t/sub/dir
+// RUN: echo > %t/sub/empty.h
+
+// Test that system header paths are expanded
+//
+// RUN: %clang -fsyntax-only -MD -MF %t.d -MT foo %s -isystem %t/sub/dir/..
+// RUN: FileCheck -check-prefix=TEST1 %s < %t.d
+// TEST1: foo:
+// TEST1: sub{{/|\\}}empty.h
+
+// Test that system header paths are not expanded to a longer form
+//
+// RUN: cd %t && %clang -fsyntax-only -MD -MF %t.d -MT foo %s -isystem sub/dir/..
+// RUN: FileCheck -check-prefix=TEST2 %s < %t.d
+// TEST2: foo:
+// TEST2: sub/dir/..{{/|\\}}empty.h
+
+// Test that user header paths are not expanded
+//
+// RUN: %clang -fsyntax-only -MD -MF %t.d -MT foo %s -I %t/sub/dir/..
+// RUN: FileCheck -check-prefix=TEST3 %s < %t.d
+// TEST3: foo:
+// TEST3: sub/dir/..{{/|\\}}empty.h
+
+// Test that system header paths are not expanded with -fno-canonical-system-headers
+// (and also that the -fsystem-system-headers option is accepted)
+//
+// RUN: %clang -fsyntax-only -MD -MF %t.d -MT foo %s -I %t/sub/dir/.. -fcanonical-system-headers -fno-canonical-system-headers
+// RUN: FileCheck -check-prefix=TEST4 %s < %t.d
+// TEST4: foo:
+// TEST4: sub/dir/..{{/|\\}}empty.h
+
+#include 
Index: lib/Tooling/ArgumentsAdjusters.cpp
===
--- lib/Tooling/ArgumentsAdjusters.cpp
+++ lib/Tooling/ArgumentsAdjusters.cpp
@@ -58,7 +58,9 @@
   StringRef Arg = Args[i];
   // All dependency-file options begin with -M. These include -MM,
   // -MF, -MG, -MP, -MT, -MQ, -MD, and -MMD.
-  if (!Arg.startswith("-M"))
+  // The exception is -f[no-]canonical-system-headers.
+  if (!Arg.startswith("-M") && Arg != "-fno-canonical-system-headers" &&
+  Arg != "-fcanonical-system-headers")
 AdjustedArgs.push_back(Args[i]);
 
   if ((Arg == "-MF") || (Arg == "-MT") || (Arg == "-MQ") ||
Index: lib/Frontend/DependencyFile.cpp
===
--- lib/Frontend/DependencyFile.cpp
+++ lib/Frontend/DependencyFile.cpp
@@ -161,6 +161,7 @@
   bool AddMissingHeaderDeps;
   bool SeenMissingHeader;
   bool IncludeModuleFiles;
+  bool CanonicalSystemHeaders;
   DependencyOutputFormat OutputFormat;
 
 private:
@@ -176,6 +177,7 @@
   AddMissingHeaderDeps(Opts.AddMissingHeaderDeps),
   SeenMissingHeader(false),
   IncludeModuleFiles(Opts.IncludeModuleFiles),
+  CanonicalSystemHeaders(Opts.CanonicalSystemHeaders),
   OutputFormat(Opts.OutputFormat) {
 for (const auto  : Opts.ExtraDeps) {
   AddFilename(ExtraDep);
@@ -288,6 +290,15 @@
   if (!FileMatchesDepCriteria(Filename.data(), FileType))
 return;
 
+  // Try to shorten system header paths like GCC does (unless
+  // -fno-canonical-system-headers is given).
+  if (CanonicalSystemHeaders && isSystem(FileType)) {
+StringRef RealPath = FE->tryGetRealPathName();
+if (!RealPath.empty() && RealPath.size() < Filename.size()) {
+  Filename = RealPath;
+}
+  }
+
   AddFilename(llvm::sys::path::remove_leading_dotslash(Filename));
 }
 
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -998,6 +998,7 @@
   Opts.Targets = Args.getAllArgValues(OPT_MT);
   Opts.IncludeSystemHeaders = Args.hasArg(OPT_sys_header_deps);
   Opts.IncludeModuleFiles = Args.hasArg(OPT_module_file_deps);
+  Opts.CanonicalSystemHeaders = !Args.hasArg(OPT_fno_canonical_system_headers);
   Opts.UsePhonyTargets = Args.hasArg(OPT_MP);
   

[PATCH] D37954: Expand absolute system header paths when using -MD depfiles

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn added a comment.

In https://reviews.llvm.org/D37954#873384, @joerg wrote:

> ninja is not the only consumer of this. For human inspection, it can be quite 
> surprising to see symlinks resolved, i.e. /usr/include/machine on NetBSD.


No problem, NetBSD disables this option by default:
https://github.com/IIJ-NetBSD/netbsd-src/blob/dd946191000153f9c8a927e5257e726879f48140/share/mk/bsd.sys.mk#L26
This option was added in 
https://github.com/IIJ-NetBSD/netbsd-src/commit/c7e9228e67fab47a6bbfb548117da93ebb20ff5c
(I am unable to find a (mailing list) discussion about the exact problem 
though.)

> Build systems have different ideas on whether they want absolute resolved 
> paths or not, so it seems like ninja should be able to cope with either 
> format. This is a lossy transformation, so I'm somewhat reluctant to agree 
> with it.

Can you be specific about the build systems you have in mind? Note:

1. GCC has added and enabled this option by default in 2012 
(https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52974). and
2. Ninja is currently slightly broken with Clang whereas it works fine with GCC.
3. To the best of my knowledge, there are no systems that break after this fix.

Looking at the GCC change, it also checks whether the canonicalized path is 
actually shorter. In that case the documented "shorten header paths" seems to 
make sense.

I thought it would be a good idea to make Clang compatible with GCC in this 
case, not sure what else could break.
If it turns out to be bad, it can be removed and/or worked around with the 
-fno-canonical-system-headers option.


https://reviews.llvm.org/D37954



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37954: Expand absolute system header paths when using -MD depfiles

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn added a comment.

In https://reviews.llvm.org/D37954#873373, @joerg wrote:

> Resolving the path doesn't necessary shorten the string at all, in many cases 
> it will be longer.


The description was based on the GCC manual page (" When preprocessing, do not 
shorten system header paths with canonicalization.")
Maybe something like:
"Do not canonicalize absolute system header paths in depfiles"

> I'm really not sure if clang should resolve symlinks like this as it can be 
> quite surprising.

This provides compatibility with GCC and fixes a bug with Ninja. At minimum, 
this canonicalization should be done when the path contains ".." components.
Can you think of a practical reason why this would cause issues?


https://reviews.llvm.org/D37954



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37954: Expand absolute system header paths when using -MD depfiles

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn added a comment.

I tried to contact Simon (the author of the GCC) patch with a question about 
the -fno-canonical-system-headers, but his Google address is bouncing.

GCC has long survived with doing this by default, I wonder if this option could 
just me omitted, enabling the feature by default.


https://reviews.llvm.org/D37954



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37954: Expand absolute system header paths when using -MD depfiles

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn created this revision.
Herald added a subscriber: klimek.

GCC defaults to expanding the dependencies in depfiles, resolving
components like ".." and symlinks. Mimic this feature to ensure that the
Ninja build tool detects the correct dependencies when a symlink changes
directory levels, see https://github.com/ninja-build/ninja/issues/1330

An option to disable this feature is added in case "these changed header
paths may conflict with some compilation environments", see
https://gcc.gnu.org/ml/gcc-patches/2012-09/msg00287.html

Note that under GCC, this option would also affect the paths shown in
the preprocessed output (-E), but that is not implemented here since I
am not sure if it is useful or breaks something else.


https://reviews.llvm.org/D37954

Files:
  include/clang/Driver/Options.td
  include/clang/Frontend/DependencyOutputOptions.h
  lib/Driver/Job.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/Frontend/DependencyFile.cpp
  lib/Tooling/ArgumentsAdjusters.cpp
  test/Preprocessor/dependencies-realpath.c

Index: test/Preprocessor/dependencies-realpath.c
===
--- /dev/null
+++ test/Preprocessor/dependencies-realpath.c
@@ -0,0 +1,32 @@
+// RUN: mkdir -p %T/sub/dir
+// RUN: echo > %T/sub/empty.h
+
+// Test that absolute system header paths are expanded in -MD
+//
+// RUN: %clang -fsyntax-only -MD -MF %t.d -MT foo %s -isystem %T/sub/dir/..
+// RUN: FileCheck -check-prefix=TEST1 %s < %t.d
+// TEST1: foo:
+// TEST1: sub{{/|\\}}empty.h
+
+// Test that relative system header paths are not expanded in -MD
+//
+// RUN: cd %T && %clang -fsyntax-only -MD -MF %t.d -MT foo %s -isystem sub/dir/..
+// RUN: FileCheck -check-prefix=TEST2 %s < %t.d
+// TEST2: foo:
+// TEST2: sub/dir/..{{/|\\}}empty.h
+
+// Test that absolute user header paths are not expanded in -MD
+//
+// RUN: cd %T && %clang -fsyntax-only -MD -MF %t.d -MT foo %s -I %T/sub/dir/..
+// RUN: FileCheck -check-prefix=TEST3 %s < %t.d
+// TEST3: foo:
+// TEST3: sub/dir/..{{/|\\}}empty.h
+
+// Test that absolute system header paths are not expanded in -MD with -fno-canonical-system-headers
+//
+// RUN: cd %T && %clang -fsyntax-only -MD -MF %t.d -MT foo %s -I %T/sub/dir/.. -fno-canonical-system-headers
+// RUN: FileCheck -check-prefix=TEST4 %s < %t.d
+// TEST4: foo:
+// TEST4: sub/dir/..{{/|\\}}empty.h
+
+#include 
Index: lib/Tooling/ArgumentsAdjusters.cpp
===
--- lib/Tooling/ArgumentsAdjusters.cpp
+++ lib/Tooling/ArgumentsAdjusters.cpp
@@ -58,7 +58,8 @@
   StringRef Arg = Args[i];
   // All dependency-file options begin with -M. These include -MM,
   // -MF, -MG, -MP, -MT, -MQ, -MD, and -MMD.
-  if (!Arg.startswith("-M"))
+  // The exception is -fno-canonical-system-headers.
+  if (!Arg.startswith("-M") && !Arg.equals("-fno-canonical-system-headers"))
 AdjustedArgs.push_back(Args[i]);
 
   if ((Arg == "-MF") || (Arg == "-MT") || (Arg == "-MQ") ||
Index: lib/Frontend/DependencyFile.cpp
===
--- lib/Frontend/DependencyFile.cpp
+++ lib/Frontend/DependencyFile.cpp
@@ -161,6 +161,7 @@
   bool AddMissingHeaderDeps;
   bool SeenMissingHeader;
   bool IncludeModuleFiles;
+  bool CanonicalSystemHeaders;
   DependencyOutputFormat OutputFormat;
 
 private:
@@ -176,6 +177,7 @@
   AddMissingHeaderDeps(Opts.AddMissingHeaderDeps),
   SeenMissingHeader(false),
   IncludeModuleFiles(Opts.IncludeModuleFiles),
+  CanonicalSystemHeaders(Opts.CanonicalSystemHeaders),
   OutputFormat(Opts.OutputFormat) {
 for (const auto  : Opts.ExtraDeps) {
   AddFilename(ExtraDep);
@@ -288,6 +290,16 @@
   if (!FileMatchesDepCriteria(Filename.data(), FileType))
 return;
 
+  // Canonicalize absolute system header paths like GCC does (unless
+  // -fno-canonical-system-headers is given).
+  if (CanonicalSystemHeaders && isSystem(FileType) &&
+  llvm::sys::path::is_absolute(Filename)) {
+StringRef RealPath = FE->tryGetRealPathName();
+if (!RealPath.empty()) {
+  Filename = RealPath;
+}
+  }
+
   AddFilename(llvm::sys::path::remove_leading_dotslash(Filename));
 }
 
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -998,6 +998,7 @@
   Opts.Targets = Args.getAllArgValues(OPT_MT);
   Opts.IncludeSystemHeaders = Args.hasArg(OPT_sys_header_deps);
   Opts.IncludeModuleFiles = Args.hasArg(OPT_module_file_deps);
+  Opts.CanonicalSystemHeaders = !Args.hasArg(OPT_fno_canonical_system_headers);
   Opts.UsePhonyTargets = Args.hasArg(OPT_MP);
   Opts.ShowHeaderIncludes = Args.hasArg(OPT_H);
   Opts.HeaderIncludeOutputFile = Args.getLastArgValue(OPT_header_include_file);
Index: lib/Driver/ToolChains/Clang.cpp

[PATCH] D33094: [ASTMatchers] Add clang-query support for equals matcher

[Peter Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL305022: [ASTMatchers] Add clang-query support for equals 
matcher (authored by Lekensteyn).

Changed prior to commit:
  https://reviews.llvm.org/D33094?vs=101817=101967#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D33094

Files:
  cfe/trunk/docs/LibASTMatchersReference.html
  cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
  cfe/trunk/lib/ASTMatchers/Dynamic/Registry.cpp
  cfe/trunk/unittests/ASTMatchers/Dynamic/RegistryTest.cpp

Index: cfe/trunk/lib/ASTMatchers/Dynamic/Registry.cpp
===
--- cfe/trunk/lib/ASTMatchers/Dynamic/Registry.cpp
+++ cfe/trunk/lib/ASTMatchers/Dynamic/Registry.cpp
@@ -56,20 +56,24 @@
   registerMatcher(#name, internal::makeMatcherAutoMarshall(\
  ::clang::ast_matchers::name, #name));
 
+#define REGISTER_MATCHER_OVERLOAD(name)\
+  registerMatcher(#name,   \
+  llvm::make_unique(name##Callbacks))
+
 #define SPECIFIC_MATCHER_OVERLOAD(name, Id)\
   static_cast<::clang::ast_matchers::name##_Type##Id>( \
   ::clang::ast_matchers::name)
 
+#define MATCHER_OVERLOAD_ENTRY(name, Id)   \
+internal::makeMatcherAutoMarshall(SPECIFIC_MATCHER_OVERLOAD(name, Id), \
+  #name)
+
 #define REGISTER_OVERLOADED_2(name)\
   do { \
-std::unique_ptr Callbacks[] = { \
-internal::makeMatcherAutoMarshall(SPECIFIC_MATCHER_OVERLOAD(name, 0),  \
-  #name),  \
-internal::makeMatcherAutoMarshall(SPECIFIC_MATCHER_OVERLOAD(name, 1),  \
-  #name)}; \
-registerMatcher(   \
-#name, \
-llvm::make_unique(Callbacks));  \
+std::unique_ptr name##Callbacks[] = {   \
+MATCHER_OVERLOAD_ENTRY(name, 0),   \
+MATCHER_OVERLOAD_ENTRY(name, 1)};  \
+REGISTER_MATCHER_OVERLOAD(name);   \
   } while (0)
 
 /// \brief Generate a registry map with all the known matchers.
@@ -83,7 +87,6 @@
   // findAll
   //
   // Other:
-  // equals
   // equalsNode
 
   REGISTER_OVERLOADED_2(callee);
@@ -96,6 +99,13 @@
   REGISTER_OVERLOADED_2(references);
   REGISTER_OVERLOADED_2(thisPointerType);
 
+  std::unique_ptr equalsCallbacks[] = {
+  MATCHER_OVERLOAD_ENTRY(equals, 0),
+  MATCHER_OVERLOAD_ENTRY(equals, 1),
+  MATCHER_OVERLOAD_ENTRY(equals, 2),
+  };
+  REGISTER_MATCHER_OVERLOAD(equals);
+
   REGISTER_MATCHER(accessSpecDecl);
   REGISTER_MATCHER(addrLabelExpr);
   REGISTER_MATCHER(alignOfExpr);
Index: cfe/trunk/unittests/ASTMatchers/Dynamic/RegistryTest.cpp
===
--- cfe/trunk/unittests/ASTMatchers/Dynamic/RegistryTest.cpp
+++ cfe/trunk/unittests/ASTMatchers/Dynamic/RegistryTest.cpp
@@ -511,6 +511,46 @@
   EXPECT_FALSE(matches("int i = 1;", Value));
 }
 
+TEST_F(RegistryTest, EqualsMatcher) {
+  Matcher BooleanStmt = constructMatcher(
+  "cxxBoolLiteral", constructMatcher("equals", VariantValue(true)))
+  .getTypedMatcher();
+  EXPECT_TRUE(matches("bool x = true;", BooleanStmt));
+  EXPECT_FALSE(matches("bool x = false;", BooleanStmt));
+  EXPECT_FALSE(matches("bool x = 0;", BooleanStmt));
+
+  BooleanStmt = constructMatcher(
+  "cxxBoolLiteral", constructMatcher("equals", VariantValue(0)))
+  .getTypedMatcher();
+  EXPECT_TRUE(matches("bool x = false;", BooleanStmt));
+  EXPECT_FALSE(matches("bool x = true;", BooleanStmt));
+  EXPECT_FALSE(matches("bool x = 0;", BooleanStmt));
+
+  Matcher DoubleStmt = constructMatcher(
+  "floatLiteral", constructMatcher("equals", VariantValue(1.2)))
+  .getTypedMatcher();
+  EXPECT_TRUE(matches("double x = 1.2;", DoubleStmt));
+  EXPECT_TRUE(matches("double x = 1.2f;", DoubleStmt));
+  EXPECT_TRUE(matches("double x = 1.2l;", DoubleStmt));
+  EXPECT_TRUE(matches("double x = 12e-1;", DoubleStmt));
+  EXPECT_FALSE(matches("double x = 1.23;", DoubleStmt));
+
+  Matcher IntegerStmt = constructMatcher(
+  "integerLiteral", constructMatcher("equals", VariantValue(42)))
+  .getTypedMatcher();
+  EXPECT_TRUE(matches("int x = 42;", IntegerStmt));
+  EXPECT_FALSE(matches("int x = 1;", IntegerStmt));
+
+  Matcher CharStmt = constructMatcher(
+  "characterLiteral", 

[PATCH] D33093: [ASTMatchers] Add support for boolean literals

[Peter Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL305020: [ASTMatchers] Add support for boolean literals 
(authored by Lekensteyn).

Changed prior to commit:
  https://reviews.llvm.org/D33093?vs=98781=101966#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D33093

Files:
  cfe/trunk/include/clang/ASTMatchers/Dynamic/Parser.h
  cfe/trunk/include/clang/ASTMatchers/Dynamic/VariantValue.h
  cfe/trunk/lib/ASTMatchers/Dynamic/Marshallers.h
  cfe/trunk/lib/ASTMatchers/Dynamic/Parser.cpp
  cfe/trunk/lib/ASTMatchers/Dynamic/VariantValue.cpp
  cfe/trunk/unittests/ASTMatchers/Dynamic/ParserTest.cpp
  cfe/trunk/unittests/ASTMatchers/Dynamic/VariantValueTest.cpp

Index: cfe/trunk/unittests/ASTMatchers/Dynamic/ParserTest.cpp
===
--- cfe/trunk/unittests/ASTMatchers/Dynamic/ParserTest.cpp
+++ cfe/trunk/unittests/ASTMatchers/Dynamic/ParserTest.cpp
@@ -75,6 +75,15 @@
   ExpectedMatchersTy ExpectedMatchers;
 };
 
+TEST(ParserTest, ParseBoolean) {
+  MockSema Sema;
+  Sema.parse("true");
+  Sema.parse("false");
+  EXPECT_EQ(2U, Sema.Values.size());
+  EXPECT_EQ(true, Sema.Values[0].getBoolean());
+  EXPECT_EQ(false, Sema.Values[1].getBoolean());
+}
+
 TEST(ParserTest, ParseUnsigned) {
   MockSema Sema;
   Sema.parse("0");
Index: cfe/trunk/unittests/ASTMatchers/Dynamic/VariantValueTest.cpp
===
--- cfe/trunk/unittests/ASTMatchers/Dynamic/VariantValueTest.cpp
+++ cfe/trunk/unittests/ASTMatchers/Dynamic/VariantValueTest.cpp
@@ -75,28 +75,40 @@
   EXPECT_TRUE(Value.isString());
   EXPECT_EQ("A", Value.getString());
   EXPECT_TRUE(Value.hasValue());
+  EXPECT_FALSE(Value.isBoolean());
   EXPECT_FALSE(Value.isUnsigned());
   EXPECT_FALSE(Value.isMatcher());
   EXPECT_EQ("String", Value.getTypeAsString());
 
   Value = VariantMatcher::SingleMatcher(recordDecl());
   EXPECT_TRUE(Value.hasValue());
+  EXPECT_FALSE(Value.isBoolean());
   EXPECT_FALSE(Value.isUnsigned());
   EXPECT_FALSE(Value.isString());
   EXPECT_TRUE(Value.isMatcher());
   EXPECT_TRUE(Value.getMatcher().hasTypedMatcher());
   EXPECT_FALSE(Value.getMatcher().hasTypedMatcher());
   EXPECT_EQ("Matcher", Value.getTypeAsString());
 
+  Value = true;
+  EXPECT_TRUE(Value.isBoolean());
+  EXPECT_EQ(true, Value.getBoolean());
+  EXPECT_TRUE(Value.hasValue());
+  EXPECT_FALSE(Value.isUnsigned());
+  EXPECT_FALSE(Value.isMatcher());
+  EXPECT_FALSE(Value.isString());
+
   Value = 17;
   EXPECT_TRUE(Value.isUnsigned());
   EXPECT_EQ(17U, Value.getUnsigned());
+  EXPECT_FALSE(Value.isBoolean());
   EXPECT_TRUE(Value.hasValue());
   EXPECT_FALSE(Value.isMatcher());
   EXPECT_FALSE(Value.isString());
 
   Value = VariantValue();
   EXPECT_FALSE(Value.hasValue());
+  EXPECT_FALSE(Value.isBoolean());
   EXPECT_FALSE(Value.isUnsigned());
   EXPECT_FALSE(Value.isString());
   EXPECT_FALSE(Value.isMatcher());
Index: cfe/trunk/include/clang/ASTMatchers/Dynamic/Parser.h
===
--- cfe/trunk/include/clang/ASTMatchers/Dynamic/Parser.h
+++ cfe/trunk/include/clang/ASTMatchers/Dynamic/Parser.h
@@ -19,8 +19,9 @@
 /// \code
 /// Grammar for the expressions supported:
 /// :=  |  | 
-///:=  | 
+///:=  |  | 
 ///  := "quoted string"
+///:= true | false
 ///   := [0-9]+
 /// := 
 ///  := () |
Index: cfe/trunk/include/clang/ASTMatchers/Dynamic/VariantValue.h
===
--- cfe/trunk/include/clang/ASTMatchers/Dynamic/VariantValue.h
+++ cfe/trunk/include/clang/ASTMatchers/Dynamic/VariantValue.h
@@ -35,6 +35,7 @@
  public:
   enum Kind {
 AK_Matcher,
+AK_Boolean,
 AK_Unsigned,
 AK_String
   };
@@ -241,6 +242,7 @@
 /// copy/assignment.
 ///
 /// Supported types:
+///  - \c bool
 ///  - \c unsigned
 ///  - \c llvm::StringRef
 ///  - \c VariantMatcher (\c DynTypedMatcher / \c Matcher)
@@ -253,14 +255,23 @@
   VariantValue =(const VariantValue );
 
   /// \brief Specific constructors for each supported type.
+  VariantValue(bool Boolean);
   VariantValue(unsigned Unsigned);
   VariantValue(StringRef String);
   VariantValue(const VariantMatcher );
 
+  /// \brief Constructs an \c unsigned value (disambiguation from bool).
+  VariantValue(int Signed) : VariantValue(static_cast(Signed)) {}
+
   /// \brief Returns true iff this is not an empty value.
   explicit operator bool() const { return hasValue(); }
   bool hasValue() const { return Type != VT_Nothing; }
 
+  /// \brief Boolean value functions.
+  bool isBoolean() const;
+  bool getBoolean() const;
+  void setBoolean(bool Boolean);
+
   /// \brief Unsigned value functions.
   bool isUnsigned() const;
   unsigned getUnsigned() const;
@@ -303,14 +314,16 @@
   /// \brief All supported value types.
   enum ValueType {
 VT_Nothing,
+VT_Boolean,
 VT_Unsigned,
 VT_String,

[PATCH] D33135: [ASTMatchers] Add support for floatLiterals

[Peter Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL305021: [ASTMatchers] Add support for floatLiterals 
(authored by Lekensteyn).

Changed prior to commit:
  https://reviews.llvm.org/D33135?vs=101816=101968#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D33135

Files:
  cfe/trunk/include/clang/ASTMatchers/Dynamic/Diagnostics.h
  cfe/trunk/include/clang/ASTMatchers/Dynamic/Parser.h
  cfe/trunk/include/clang/ASTMatchers/Dynamic/VariantValue.h
  cfe/trunk/lib/ASTMatchers/Dynamic/Diagnostics.cpp
  cfe/trunk/lib/ASTMatchers/Dynamic/Marshallers.h
  cfe/trunk/lib/ASTMatchers/Dynamic/Parser.cpp
  cfe/trunk/lib/ASTMatchers/Dynamic/VariantValue.cpp
  cfe/trunk/unittests/ASTMatchers/Dynamic/ParserTest.cpp
  cfe/trunk/unittests/ASTMatchers/Dynamic/VariantValueTest.cpp

Index: cfe/trunk/include/clang/ASTMatchers/Dynamic/VariantValue.h
===
--- cfe/trunk/include/clang/ASTMatchers/Dynamic/VariantValue.h
+++ cfe/trunk/include/clang/ASTMatchers/Dynamic/VariantValue.h
@@ -36,6 +36,7 @@
   enum Kind {
 AK_Matcher,
 AK_Boolean,
+AK_Double,
 AK_Unsigned,
 AK_String
   };
@@ -243,6 +244,7 @@
 ///
 /// Supported types:
 ///  - \c bool
+//   - \c double
 ///  - \c unsigned
 ///  - \c llvm::StringRef
 ///  - \c VariantMatcher (\c DynTypedMatcher / \c Matcher)
@@ -256,6 +258,7 @@
 
   /// \brief Specific constructors for each supported type.
   VariantValue(bool Boolean);
+  VariantValue(double Double);
   VariantValue(unsigned Unsigned);
   VariantValue(StringRef String);
   VariantValue(const VariantMatcher );
@@ -272,6 +275,11 @@
   bool getBoolean() const;
   void setBoolean(bool Boolean);
 
+  /// \brief Double value functions.
+  bool isDouble() const;
+  double getDouble() const;
+  void setDouble(double Double);
+
   /// \brief Unsigned value functions.
   bool isUnsigned() const;
   unsigned getUnsigned() const;
@@ -315,14 +323,16 @@
   enum ValueType {
 VT_Nothing,
 VT_Boolean,
+VT_Double,
 VT_Unsigned,
 VT_String,
 VT_Matcher
   };
 
   /// \brief All supported value types.
   union AllValues {
 unsigned Unsigned;
+double Double;
 bool Boolean;
 std::string *String;
 VariantMatcher *Matcher;
Index: cfe/trunk/include/clang/ASTMatchers/Dynamic/Parser.h
===
--- cfe/trunk/include/clang/ASTMatchers/Dynamic/Parser.h
+++ cfe/trunk/include/clang/ASTMatchers/Dynamic/Parser.h
@@ -19,9 +19,10 @@
 /// \code
 /// Grammar for the expressions supported:
 /// :=  |  | 
-///:=  |  | 
+///:=  |  |  | 
 ///  := "quoted string"
 ///:= true | false
+/// := [0-9]+.[0-9]* | [0-9]+.[0-9]*[eE][-+]?[0-9]+
 ///   := [0-9]+
 /// := 
 ///  := () |
Index: cfe/trunk/include/clang/ASTMatchers/Dynamic/Diagnostics.h
===
--- cfe/trunk/include/clang/ASTMatchers/Dynamic/Diagnostics.h
+++ cfe/trunk/include/clang/ASTMatchers/Dynamic/Diagnostics.h
@@ -76,7 +76,7 @@
 ET_ParserInvalidToken = 106,
 ET_ParserMalformedBindExpr = 107,
 ET_ParserTrailingCode = 108,
-ET_ParserUnsignedError = 109,
+ET_ParserNumberError = 109,
 ET_ParserOverloadedType = 110
   };
 
Index: cfe/trunk/lib/ASTMatchers/Dynamic/Marshallers.h
===
--- cfe/trunk/lib/ASTMatchers/Dynamic/Marshallers.h
+++ cfe/trunk/lib/ASTMatchers/Dynamic/Marshallers.h
@@ -75,6 +75,16 @@
   }
 };
 
+template <> struct ArgTypeTraits {
+  static bool is(const VariantValue ) { return Value.isDouble(); }
+  static double get(const VariantValue ) {
+return Value.getDouble();
+  }
+  static ArgKind getKind() {
+return ArgKind(ArgKind::AK_Double);
+  }
+};
+
 template <> struct ArgTypeTraits {
   static bool is(const VariantValue ) { return Value.isUnsigned(); }
   static unsigned get(const VariantValue ) {
Index: cfe/trunk/lib/ASTMatchers/Dynamic/VariantValue.cpp
===
--- cfe/trunk/lib/ASTMatchers/Dynamic/VariantValue.cpp
+++ cfe/trunk/lib/ASTMatchers/Dynamic/VariantValue.cpp
@@ -26,6 +26,8 @@
 return (Twine("Matcher<") + MatcherKind.asStringRef() + ">").str();
   case AK_Boolean:
 return "boolean";
+  case AK_Double:
+return "double";
   case AK_Unsigned:
 return "unsigned";
   case AK_String:
@@ -253,6 +255,10 @@
   setBoolean(Boolean);
 }
 
+VariantValue::VariantValue(double Double) : Type(VT_Nothing) {
+  setDouble(Double);
+}
+
 VariantValue::VariantValue(unsigned Unsigned) : Type(VT_Nothing) {
   setUnsigned(Unsigned);
 }
@@ -274,6 +280,9 @@
   case VT_Boolean:
 setBoolean(Other.getBoolean());
 break;
+  case VT_Double:
+setDouble(Other.getDouble());
+break;
   case VT_Unsigned:
 setUnsigned(Other.getUnsigned());
 break;
@@ -300,6 +309,7 @@
 

[PATCH] D33094: [ASTMatchers] Add clang-query support for equals matcher

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn added inline comments.



Comment at: unittests/ASTMatchers/Dynamic/RegistryTest.cpp:545
+  .getTypedMatcher();
+  EXPECT_TRUE(matches("int x = 'x';", CharStmt));
+  EXPECT_FALSE(matches("int x = 120;", CharStmt));

Lekensteyn wrote:
> aaron.ballman wrote:
> > Can you add some tests involving the other character literal types (L, u, 
> > U, u8)?
> will do
Done (except for u8 which is only defined for string literals).


https://reviews.llvm.org/D33094



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33094: [ASTMatchers] Add clang-query support for equals matcher

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn updated this revision to Diff 101817.
Lekensteyn marked 7 inline comments as done.
Lekensteyn added a comment.

diff from previous patch:

  diff --git a/unittests/ASTMatchers/Dynamic/RegistryTest.cpp 
b/unittests/ASTMatchers/Dynamic/RegistryTest.cpp
  index 29fcdec6c1..84e31f721a 100644
  --- a/unittests/ASTMatchers/Dynamic/RegistryTest.cpp
  +++ b/unittests/ASTMatchers/Dynamic/RegistryTest.cpp
  @@ -530,6 +530,8 @@ TEST_F(RegistryTest, EqualsMatcher) {
 "floatLiteral", constructMatcher("equals", VariantValue(1.2)))
 .getTypedMatcher();
 EXPECT_TRUE(matches("double x = 1.2;", DoubleStmt));
  +  EXPECT_TRUE(matches("double x = 1.2f;", DoubleStmt));
  +  EXPECT_TRUE(matches("double x = 1.2l;", DoubleStmt));
 EXPECT_TRUE(matches("double x = 12e-1;", DoubleStmt));
 EXPECT_FALSE(matches("double x = 1.23;", DoubleStmt));
   
  @@ -543,6 +545,9 @@ TEST_F(RegistryTest, EqualsMatcher) {
 "characterLiteral", constructMatcher("equals", VariantValue('x')))
 .getTypedMatcher();
 EXPECT_TRUE(matches("int x = 'x';", CharStmt));
  +  EXPECT_TRUE(matches("int x = L'x';", CharStmt));
  +  EXPECT_TRUE(matches("int x = u'x';", CharStmt));
  +  EXPECT_TRUE(matches("int x = U'x';", CharStmt));
 EXPECT_FALSE(matches("int x = 120;", CharStmt));
   }


https://reviews.llvm.org/D33094

Files:
  docs/LibASTMatchersReference.html
  include/clang/ASTMatchers/ASTMatchers.h
  lib/ASTMatchers/Dynamic/Registry.cpp
  unittests/ASTMatchers/Dynamic/RegistryTest.cpp

Index: unittests/ASTMatchers/Dynamic/RegistryTest.cpp
===
--- unittests/ASTMatchers/Dynamic/RegistryTest.cpp
+++ unittests/ASTMatchers/Dynamic/RegistryTest.cpp
@@ -511,6 +511,46 @@
   EXPECT_FALSE(matches("int i = 1;", Value));
 }
 
+TEST_F(RegistryTest, EqualsMatcher) {
+  Matcher BooleanStmt = constructMatcher(
+  "cxxBoolLiteral", constructMatcher("equals", VariantValue(true)))
+  .getTypedMatcher();
+  EXPECT_TRUE(matches("bool x = true;", BooleanStmt));
+  EXPECT_FALSE(matches("bool x = false;", BooleanStmt));
+  EXPECT_FALSE(matches("bool x = 0;", BooleanStmt));
+
+  BooleanStmt = constructMatcher(
+  "cxxBoolLiteral", constructMatcher("equals", VariantValue(0)))
+  .getTypedMatcher();
+  EXPECT_TRUE(matches("bool x = false;", BooleanStmt));
+  EXPECT_FALSE(matches("bool x = true;", BooleanStmt));
+  EXPECT_FALSE(matches("bool x = 0;", BooleanStmt));
+
+  Matcher DoubleStmt = constructMatcher(
+  "floatLiteral", constructMatcher("equals", VariantValue(1.2)))
+  .getTypedMatcher();
+  EXPECT_TRUE(matches("double x = 1.2;", DoubleStmt));
+  EXPECT_TRUE(matches("double x = 1.2f;", DoubleStmt));
+  EXPECT_TRUE(matches("double x = 1.2l;", DoubleStmt));
+  EXPECT_TRUE(matches("double x = 12e-1;", DoubleStmt));
+  EXPECT_FALSE(matches("double x = 1.23;", DoubleStmt));
+
+  Matcher IntegerStmt = constructMatcher(
+  "integerLiteral", constructMatcher("equals", VariantValue(42)))
+  .getTypedMatcher();
+  EXPECT_TRUE(matches("int x = 42;", IntegerStmt));
+  EXPECT_FALSE(matches("int x = 1;", IntegerStmt));
+
+  Matcher CharStmt = constructMatcher(
+  "characterLiteral", constructMatcher("equals", VariantValue('x')))
+  .getTypedMatcher();
+  EXPECT_TRUE(matches("int x = 'x';", CharStmt));
+  EXPECT_TRUE(matches("int x = L'x';", CharStmt));
+  EXPECT_TRUE(matches("int x = u'x';", CharStmt));
+  EXPECT_TRUE(matches("int x = U'x';", CharStmt));
+  EXPECT_FALSE(matches("int x = 120;", CharStmt));
+}
+
 } // end anonymous namespace
 } // end namespace dynamic
 } // end namespace ast_matchers
Index: lib/ASTMatchers/Dynamic/Registry.cpp
===
--- lib/ASTMatchers/Dynamic/Registry.cpp
+++ lib/ASTMatchers/Dynamic/Registry.cpp
@@ -56,20 +56,24 @@
   registerMatcher(#name, internal::makeMatcherAutoMarshall(\
  ::clang::ast_matchers::name, #name));
 
+#define REGISTER_MATCHER_OVERLOAD(name)\
+  registerMatcher(#name,   \
+  llvm::make_unique(name##Callbacks))
+
 #define SPECIFIC_MATCHER_OVERLOAD(name, Id)\
   static_cast<::clang::ast_matchers::name##_Type##Id>( \
   ::clang::ast_matchers::name)
 
+#define MATCHER_OVERLOAD_ENTRY(name, Id)   \
+internal::makeMatcherAutoMarshall(SPECIFIC_MATCHER_OVERLOAD(name, Id), \
+  #name)
+
 #define REGISTER_OVERLOADED_2(name)\
   do { \
-std::unique_ptr Callbacks[] = { \
-internal::makeMatcherAutoMarshall(SPECIFIC_MATCHER_OVERLOAD(name, 0),  \
-  #name), 

[PATCH] D33135: [ASTMatchers] Add support for floatLiterals

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn added a comment.

Rebased patches on latest clang master (trunk), there were no changes in 
ASTMatchers.
boolean literal patch was unchanged, this floating literal patch was updated to 
address comments.




Comment at: include/clang/ASTMatchers/Dynamic/VariantValue.h:335
 unsigned Unsigned;
+double Double;
 bool Boolean;

aaron.ballman wrote:
> Lekensteyn wrote:
> > aaron.ballman wrote:
> > > Lekensteyn wrote:
> > > > aaron.ballman wrote:
> > > > > This may or may not be a good idea, but do we want to put the values 
> > > > > into an APFloat rather than a double? My concern with double is that 
> > > > > (0) it may be subtly different if the user wants a 16- or 32-bit 
> > > > > float explicitly, (1) it won't be able to represent long double 
> > > > > values, or quad double.
> > > > > 
> > > > > I'm thinking this value could be passed directly from the C++ API as 
> > > > > an APFloat, float, or double, or provided using a StringRef for the 
> > > > > dynamic API.
> > > > (32-bit) double values are a superset of (16-bit) float values, that 
> > > > should be OK.
> > > > Long doubles are possible in the AST (e.g. for `0.1L`), but neither C11 
> > > > nor C++14 seem to define a quad double literal type (so that should be 
> > > > of a lesser concern).
> > > > 
> > > > Reasons why I chose for double instead of APFloat:
> > > > - `strtod` is readily available and does not abort the program. By 
> > > > contrast, `APFloat(StringRef)` trips on assertions if the input is 
> > > > invalid.
> > > > - I was not sure if the APFloat class can be used in an union.
> > > The downside to using `strtod()` is that invalid input is silently 
> > > accepted. However, assertions on invalid input is certainly not good 
> > > either. It might be worth modifying `APFloat::convertFromString()` to 
> > > accept invalid input and return an error.
> > > 
> > > I think instead of an `APFloat`, maybe using an `APValue` for both the 
> > > `Unsigned` and `Double` fields might work. At the very least, it should 
> > > give you implementation ideas.
> > > 
> > > There is a quad double literal suffix: `q`. It's only supported on some 
> > > architectures, however. There are also imaginary numbers (`i`) and half 
> > > (`h`).
> > The strtod conversion was based on parseDouble in 
> > lib/Support/CommandLine.cpp, so any conversion issues also exist there.
> > 
> > Same question, can APFloat/APValue be used in a union?
> > 
> > float (or quad-double suffixes) are explicitly not supported now in this 
> > matcher, maybe they can be added later but for now I decided to keep the 
> > grammar simple (that is, do not express double/float data types via the 
> > literal).
> > The strtod conversion was based on parseDouble in 
> > lib/Support/CommandLine.cpp, so any conversion issues also exist there.
> 
> Good to know.
> 
> > Same question, can APFloat/APValue be used in a union?
> 
> I believe so, but I've not tried it myself. Also, as I mentioned, `APValue` 
> demonstrates another implementation strategy in case you cannot use a union 
> directly.
> 
> > float (or quad-double suffixes) are explicitly not supported now in this 
> > matcher, maybe they can be added later but for now I decided to keep the 
> > grammar simple (that is, do not express double/float data types via the 
> > literal).
> 
> That's reasonable for an initial implementation.
I think I'll keep it like this for now and defer eventual conversion to APValue 
for a future patch that also makes uint64_t possible. Is that OK?


https://reviews.llvm.org/D33135



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33135: [ASTMatchers] Add support for floatLiterals

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn updated this revision to Diff 101816.
Lekensteyn marked 8 inline comments as done.
Lekensteyn added a comment.

diff from previous version:

  diff --git a/include/clang/ASTMatchers/Dynamic/Parser.h 
b/include/clang/ASTMatchers/Dynamic/Parser.h
  index 0d0c2ba540..5ec4a9abf4 100644
  --- a/include/clang/ASTMatchers/Dynamic/Parser.h
  +++ b/include/clang/ASTMatchers/Dynamic/Parser.h
  @@ -22,7 +22,7 @@
   ///:=  |  |  | 

   ///  := "quoted string"
   ///:= true | false
  -/// := 1.0 | 2e-3 | 3.45e67
  +/// := [0-9]+.[0-9]* | [0-9]+.[0-9]*[eE][-+]?[0-9]+
   ///   := [0-9]+
   /// := 
   ///  := () |
  diff --git a/lib/ASTMatchers/Dynamic/Parser.cpp 
b/lib/ASTMatchers/Dynamic/Parser.cpp
  index 669e5ca44f..ff5c5fb657 100644
  --- a/lib/ASTMatchers/Dynamic/Parser.cpp
  +++ b/lib/ASTMatchers/Dynamic/Parser.cpp
  @@ -130,8 +130,8 @@ private:
   
   case '0': case '1': case '2': case '3': case '4':
   case '5': case '6': case '7': case '8': case '9':
  -  // Parse an unsigned literal.
  -  consumeUnsignedLiteral();
  +  // Parse an unsigned and float literal.
  +  consumeNumberLiteral();
 break;
   
   default:
  @@ -176,8 +176,8 @@ private:
   return Result;
 }
   
  -  /// \brief Consume an unsigned literal.
  -  void consumeUnsignedLiteral(TokenInfo *Result) {
  +  /// \brief Consume an unsigned and float literal.
  +  void consumeNumberLiteral(TokenInfo *Result) {
   bool isFloatingLiteral = false;
   unsigned Length = 1;
   if (Code.size() > 1) {
  @@ -205,8 +205,9 @@ private:
   
   if (isFloatingLiteral) {
 char *end;
  +  errno = 0;
 double doubleValue = strtod(Result->Text.str().c_str(), );
  -  if (*end == 0) {
  +  if (*end == 0 && errno == 0) {
   Result->Kind = TokenInfo::TK_Literal;
   Result->Value = doubleValue;
   return;


https://reviews.llvm.org/D33135

Files:
  include/clang/ASTMatchers/Dynamic/Diagnostics.h
  include/clang/ASTMatchers/Dynamic/Parser.h
  include/clang/ASTMatchers/Dynamic/VariantValue.h
  lib/ASTMatchers/Dynamic/Diagnostics.cpp
  lib/ASTMatchers/Dynamic/Marshallers.h
  lib/ASTMatchers/Dynamic/Parser.cpp
  lib/ASTMatchers/Dynamic/VariantValue.cpp
  unittests/ASTMatchers/Dynamic/ParserTest.cpp
  unittests/ASTMatchers/Dynamic/VariantValueTest.cpp

Index: unittests/ASTMatchers/Dynamic/VariantValueTest.cpp
===
--- unittests/ASTMatchers/Dynamic/VariantValueTest.cpp
+++ unittests/ASTMatchers/Dynamic/VariantValueTest.cpp
@@ -76,13 +76,15 @@
   EXPECT_EQ("A", Value.getString());
   EXPECT_TRUE(Value.hasValue());
   EXPECT_FALSE(Value.isBoolean());
+  EXPECT_FALSE(Value.isDouble());
   EXPECT_FALSE(Value.isUnsigned());
   EXPECT_FALSE(Value.isMatcher());
   EXPECT_EQ("String", Value.getTypeAsString());
 
   Value = VariantMatcher::SingleMatcher(recordDecl());
   EXPECT_TRUE(Value.hasValue());
   EXPECT_FALSE(Value.isBoolean());
+  EXPECT_FALSE(Value.isDouble());
   EXPECT_FALSE(Value.isUnsigned());
   EXPECT_FALSE(Value.isString());
   EXPECT_TRUE(Value.isMatcher());
@@ -98,17 +100,28 @@
   EXPECT_FALSE(Value.isMatcher());
   EXPECT_FALSE(Value.isString());
 
+  Value = 3.14;
+  EXPECT_TRUE(Value.isDouble());
+  EXPECT_EQ(3.14, Value.getDouble());
+  EXPECT_TRUE(Value.hasValue());
+  EXPECT_FALSE(Value.isBoolean());
+  EXPECT_FALSE(Value.isUnsigned());
+  EXPECT_FALSE(Value.isMatcher());
+  EXPECT_FALSE(Value.isString());
+
   Value = 17;
   EXPECT_TRUE(Value.isUnsigned());
   EXPECT_EQ(17U, Value.getUnsigned());
   EXPECT_FALSE(Value.isBoolean());
+  EXPECT_FALSE(Value.isDouble());
   EXPECT_TRUE(Value.hasValue());
   EXPECT_FALSE(Value.isMatcher());
   EXPECT_FALSE(Value.isString());
 
   Value = VariantValue();
   EXPECT_FALSE(Value.hasValue());
   EXPECT_FALSE(Value.isBoolean());
+  EXPECT_FALSE(Value.isDouble());
   EXPECT_FALSE(Value.isUnsigned());
   EXPECT_FALSE(Value.isString());
   EXPECT_FALSE(Value.isMatcher());
Index: unittests/ASTMatchers/Dynamic/ParserTest.cpp
===
--- unittests/ASTMatchers/Dynamic/ParserTest.cpp
+++ unittests/ASTMatchers/Dynamic/ParserTest.cpp
@@ -84,6 +84,21 @@
   EXPECT_EQ(false, Sema.Values[1].getBoolean());
 }
 
+TEST(ParserTest, ParseDouble) {
+  MockSema Sema;
+  Sema.parse("1.0");
+  Sema.parse("2.0f");
+  Sema.parse("34.56e-78");
+  Sema.parse("4.E+6");
+  Sema.parse("1");
+  EXPECT_EQ(5U, Sema.Values.size());
+  EXPECT_EQ(1.0, Sema.Values[0].getDouble());
+  EXPECT_EQ("1:1: Error parsing numeric literal: <2.0f>", Sema.Errors[1]);
+  EXPECT_EQ(34.56e-78, Sema.Values[2].getDouble());
+  EXPECT_EQ(4e+6, Sema.Values[3].getDouble());
+  EXPECT_FALSE(Sema.Values[4].isDouble());
+}
+
 TEST(ParserTest, ParseUnsigned) {
   MockSema Sema;
   Sema.parse("0");
@@ -95,8 +110,8 @@
   EXPECT_EQ(0U, Sema.Values[0].getUnsigned());
   

[PATCH] D33135: [ASTMatchers] Add support for floatLiterals

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn added inline comments.



Comment at: include/clang/ASTMatchers/Dynamic/VariantValue.h:335
 unsigned Unsigned;
+double Double;
 bool Boolean;

aaron.ballman wrote:
> Lekensteyn wrote:
> > aaron.ballman wrote:
> > > This may or may not be a good idea, but do we want to put the values into 
> > > an APFloat rather than a double? My concern with double is that (0) it 
> > > may be subtly different if the user wants a 16- or 32-bit float 
> > > explicitly, (1) it won't be able to represent long double values, or quad 
> > > double.
> > > 
> > > I'm thinking this value could be passed directly from the C++ API as an 
> > > APFloat, float, or double, or provided using a StringRef for the dynamic 
> > > API.
> > (32-bit) double values are a superset of (16-bit) float values, that should 
> > be OK.
> > Long doubles are possible in the AST (e.g. for `0.1L`), but neither C11 nor 
> > C++14 seem to define a quad double literal type (so that should be of a 
> > lesser concern).
> > 
> > Reasons why I chose for double instead of APFloat:
> > - `strtod` is readily available and does not abort the program. By 
> > contrast, `APFloat(StringRef)` trips on assertions if the input is invalid.
> > - I was not sure if the APFloat class can be used in an union.
> The downside to using `strtod()` is that invalid input is silently accepted. 
> However, assertions on invalid input is certainly not good either. It might 
> be worth modifying `APFloat::convertFromString()` to accept invalid input and 
> return an error.
> 
> I think instead of an `APFloat`, maybe using an `APValue` for both the 
> `Unsigned` and `Double` fields might work. At the very least, it should give 
> you implementation ideas.
> 
> There is a quad double literal suffix: `q`. It's only supported on some 
> architectures, however. There are also imaginary numbers (`i`) and half (`h`).
The strtod conversion was based on parseDouble in lib/Support/CommandLine.cpp, 
so any conversion issues also exist there.

Same question, can APFloat/APValue be used in a union?

float (or quad-double suffixes) are explicitly not supported now in this 
matcher, maybe they can be added later but for now I decided to keep the 
grammar simple (that is, do not express double/float data types via the 
literal).



Comment at: lib/ASTMatchers/Dynamic/Parser.cpp:209
+  double doubleValue = strtod(Result->Text.str().c_str(), );
+  if (*end == 0) {
+Result->Kind = TokenInfo::TK_Literal;

aaron.ballman wrote:
> You're failing to check errno here to ensure the value is actually valid.
will check this later, see also the previous comment


https://reviews.llvm.org/D33135



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33094: [ASTMatchers] Add clang-query support for equals matcher

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn added inline comments.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:3846
+  CXXBoolLiteralExpr,
+  IntegerLiteral),
+  unsigned, Value, 1) {

aaron.ballman wrote:
> Is there a reason to not allow the equals matcher to do something like 
> `floatingLiteral(equals(1))`? Sure, the user could always write `1.0`, but it 
> seems somewhat hostile to require it.
The ValueMatcher for float does not accept integers at the moment, adding 
FloatingLiteral now results in a compile error. It can be added though (might 
do this as well in the next revision, either in the existing patches or a new 
one).



Comment at: unittests/ASTMatchers/Dynamic/RegistryTest.cpp:534
+  EXPECT_TRUE(matches("double x = 12e-1;", DoubleStmt));
+  EXPECT_FALSE(matches("double x = 1.23;", DoubleStmt));
+

aaron.ballman wrote:
> Can you add tests for floating literals with suffixes (f, l)?
will do



Comment at: unittests/ASTMatchers/Dynamic/RegistryTest.cpp:545
+  .getTypedMatcher();
+  EXPECT_TRUE(matches("int x = 'x';", CharStmt));
+  EXPECT_FALSE(matches("int x = 120;", CharStmt));

aaron.ballman wrote:
> Can you add some tests involving the other character literal types (L, u, U, 
> u8)?
will do


https://reviews.llvm.org/D33094



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33094: [ASTMatchers] Add clang-query support for equals matcher

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn updated this revision to Diff 98917.
Lekensteyn marked 9 inline comments as done.
Lekensteyn retitled this revision from "[ASTMatchers] Add equals support for 
integer and boolean literals" to "[ASTMatchers] Add clang-query support for 
equals matcher".
Lekensteyn edited the summary of this revision.
Lekensteyn added a comment.

v1 -> v2:

- Add CharacterLiteral and FloatingLiterals support
- updated documentation comment to include examples for all four supported 
matcher types
- updated docs with dump_ast_matchers.py

Depends on:

- https://reviews.llvm.org/D33093 for boolean support
- https://reviews.llvm.org/D33135 for float support


https://reviews.llvm.org/D33094

Files:
  docs/LibASTMatchersReference.html
  include/clang/ASTMatchers/ASTMatchers.h
  lib/ASTMatchers/Dynamic/Registry.cpp
  unittests/ASTMatchers/Dynamic/RegistryTest.cpp

Index: unittests/ASTMatchers/Dynamic/RegistryTest.cpp
===
--- unittests/ASTMatchers/Dynamic/RegistryTest.cpp
+++ unittests/ASTMatchers/Dynamic/RegistryTest.cpp
@@ -511,6 +511,41 @@
   EXPECT_FALSE(matches("int i = 1;", Value));
 }
 
+TEST_F(RegistryTest, EqualsMatcher) {
+  Matcher BooleanStmt = constructMatcher(
+  "cxxBoolLiteral", constructMatcher("equals", VariantValue(true)))
+  .getTypedMatcher();
+  EXPECT_TRUE(matches("bool x = true;", BooleanStmt));
+  EXPECT_FALSE(matches("bool x = false;", BooleanStmt));
+  EXPECT_FALSE(matches("bool x = 0;", BooleanStmt));
+
+  BooleanStmt = constructMatcher(
+  "cxxBoolLiteral", constructMatcher("equals", VariantValue(0)))
+  .getTypedMatcher();
+  EXPECT_TRUE(matches("bool x = false;", BooleanStmt));
+  EXPECT_FALSE(matches("bool x = true;", BooleanStmt));
+  EXPECT_FALSE(matches("bool x = 0;", BooleanStmt));
+
+  Matcher DoubleStmt = constructMatcher(
+  "floatLiteral", constructMatcher("equals", VariantValue(1.2)))
+  .getTypedMatcher();
+  EXPECT_TRUE(matches("double x = 1.2;", DoubleStmt));
+  EXPECT_TRUE(matches("double x = 12e-1;", DoubleStmt));
+  EXPECT_FALSE(matches("double x = 1.23;", DoubleStmt));
+
+  Matcher IntegerStmt = constructMatcher(
+  "integerLiteral", constructMatcher("equals", VariantValue(42)))
+  .getTypedMatcher();
+  EXPECT_TRUE(matches("int x = 42;", IntegerStmt));
+  EXPECT_FALSE(matches("int x = 1;", IntegerStmt));
+
+  Matcher CharStmt = constructMatcher(
+  "characterLiteral", constructMatcher("equals", VariantValue('x')))
+  .getTypedMatcher();
+  EXPECT_TRUE(matches("int x = 'x';", CharStmt));
+  EXPECT_FALSE(matches("int x = 120;", CharStmt));
+}
+
 } // end anonymous namespace
 } // end namespace dynamic
 } // end namespace ast_matchers
Index: lib/ASTMatchers/Dynamic/Registry.cpp
===
--- lib/ASTMatchers/Dynamic/Registry.cpp
+++ lib/ASTMatchers/Dynamic/Registry.cpp
@@ -56,20 +56,24 @@
   registerMatcher(#name, internal::makeMatcherAutoMarshall(\
  ::clang::ast_matchers::name, #name));
 
+#define REGISTER_MATCHER_OVERLOAD(name)\
+  registerMatcher(#name,   \
+  llvm::make_unique(name##Callbacks))
+
 #define SPECIFIC_MATCHER_OVERLOAD(name, Id)\
   static_cast<::clang::ast_matchers::name##_Type##Id>( \
   ::clang::ast_matchers::name)
 
+#define MATCHER_OVERLOAD_ENTRY(name, Id)   \
+internal::makeMatcherAutoMarshall(SPECIFIC_MATCHER_OVERLOAD(name, Id), \
+  #name)
+
 #define REGISTER_OVERLOADED_2(name)\
   do { \
-std::unique_ptr Callbacks[] = { \
-internal::makeMatcherAutoMarshall(SPECIFIC_MATCHER_OVERLOAD(name, 0),  \
-  #name),  \
-internal::makeMatcherAutoMarshall(SPECIFIC_MATCHER_OVERLOAD(name, 1),  \
-  #name)}; \
-registerMatcher(   \
-#name, \
-llvm::make_unique(Callbacks));  \
+std::unique_ptr name##Callbacks[] = {   \
+MATCHER_OVERLOAD_ENTRY(name, 0),   \
+MATCHER_OVERLOAD_ENTRY(name, 1)};  \
+REGISTER_MATCHER_OVERLOAD(name);   \
   } while (0)
 
 /// \brief Generate a registry map with all the known matchers.
@@ -83,7 +87,6 @@
   // findAll
   //
   // Other:
-  // equals
   // equalsNode
 
   REGISTER_OVERLOADED_2(callee);
@@ -96,6 +99,13 

[PATCH] D33135: [ASTMatchers] Add support for floatLiterals

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn added a comment.

By the way, I think that `long double` is less common than long unsigned 
literals, so changing unsigned to uint64_t might be something more important?




Comment at: include/clang/ASTMatchers/Dynamic/Parser.h:25
 ///:= true | false
+/// := 1.0 | 2e-3 | 3.45e67
 ///   := [0-9]+

aaron.ballman wrote:
> It'd be good to list the actual grammar rather than a few examples.
I am concerned that listing a very precise grammar actually makes this less 
readable (see also the StringLiteral example).

If a grammar is used instead, how about this:

 := [0-9]+.[0-9]* | [0-9]+.[0-9]*[eE][-+]?[0-9]+




Comment at: include/clang/ASTMatchers/Dynamic/VariantValue.h:335
 unsigned Unsigned;
+double Double;
 bool Boolean;

aaron.ballman wrote:
> This may or may not be a good idea, but do we want to put the values into an 
> APFloat rather than a double? My concern with double is that (0) it may be 
> subtly different if the user wants a 16- or 32-bit float explicitly, (1) it 
> won't be able to represent long double values, or quad double.
> 
> I'm thinking this value could be passed directly from the C++ API as an 
> APFloat, float, or double, or provided using a StringRef for the dynamic API.
(32-bit) double values are a superset of (16-bit) float values, that should be 
OK.
Long doubles are possible in the AST (e.g. for `0.1L`), but neither C11 nor 
C++14 seem to define a quad double literal type (so that should be of a lesser 
concern).

Reasons why I chose for double instead of APFloat:
- `strtod` is readily available and does not abort the program. By contrast, 
`APFloat(StringRef)` trips on assertions if the input is invalid.
- I was not sure if the APFloat class can be used in an union.



Comment at: lib/ASTMatchers/Dynamic/Parser.cpp:180
   /// \brief Consume an unsigned literal.
   void consumeUnsignedLiteral(TokenInfo *Result) {
+bool isFloatingLiteral = false;

aaron.ballman wrote:
> This function should be renamed and the comment updated.
How does "consumeNumberLiteral" sound?


https://reviews.llvm.org/D33135



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33135: [ASTMatchers] Add support for floatLiterals

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn created this revision.

Needed to support something like "floatLiteral(equals(1.0))". The
parser for floating point numbers is kept simple, so instead of ".1" you
have to use "0.1".


https://reviews.llvm.org/D33135

Files:
  include/clang/ASTMatchers/Dynamic/Diagnostics.h
  include/clang/ASTMatchers/Dynamic/Parser.h
  include/clang/ASTMatchers/Dynamic/VariantValue.h
  lib/ASTMatchers/Dynamic/Diagnostics.cpp
  lib/ASTMatchers/Dynamic/Marshallers.h
  lib/ASTMatchers/Dynamic/Parser.cpp
  lib/ASTMatchers/Dynamic/VariantValue.cpp
  unittests/ASTMatchers/Dynamic/ParserTest.cpp
  unittests/ASTMatchers/Dynamic/VariantValueTest.cpp

Index: unittests/ASTMatchers/Dynamic/VariantValueTest.cpp
===
--- unittests/ASTMatchers/Dynamic/VariantValueTest.cpp
+++ unittests/ASTMatchers/Dynamic/VariantValueTest.cpp
@@ -76,13 +76,15 @@
   EXPECT_EQ("A", Value.getString());
   EXPECT_TRUE(Value.hasValue());
   EXPECT_FALSE(Value.isBoolean());
+  EXPECT_FALSE(Value.isDouble());
   EXPECT_FALSE(Value.isUnsigned());
   EXPECT_FALSE(Value.isMatcher());
   EXPECT_EQ("String", Value.getTypeAsString());
 
   Value = VariantMatcher::SingleMatcher(recordDecl());
   EXPECT_TRUE(Value.hasValue());
   EXPECT_FALSE(Value.isBoolean());
+  EXPECT_FALSE(Value.isDouble());
   EXPECT_FALSE(Value.isUnsigned());
   EXPECT_FALSE(Value.isString());
   EXPECT_TRUE(Value.isMatcher());
@@ -98,17 +100,28 @@
   EXPECT_FALSE(Value.isMatcher());
   EXPECT_FALSE(Value.isString());
 
+  Value = 3.14;
+  EXPECT_TRUE(Value.isDouble());
+  EXPECT_EQ(3.14, Value.getDouble());
+  EXPECT_TRUE(Value.hasValue());
+  EXPECT_FALSE(Value.isBoolean());
+  EXPECT_FALSE(Value.isUnsigned());
+  EXPECT_FALSE(Value.isMatcher());
+  EXPECT_FALSE(Value.isString());
+
   Value = 17;
   EXPECT_TRUE(Value.isUnsigned());
   EXPECT_EQ(17U, Value.getUnsigned());
   EXPECT_FALSE(Value.isBoolean());
+  EXPECT_FALSE(Value.isDouble());
   EXPECT_TRUE(Value.hasValue());
   EXPECT_FALSE(Value.isMatcher());
   EXPECT_FALSE(Value.isString());
 
   Value = VariantValue();
   EXPECT_FALSE(Value.hasValue());
   EXPECT_FALSE(Value.isBoolean());
+  EXPECT_FALSE(Value.isDouble());
   EXPECT_FALSE(Value.isUnsigned());
   EXPECT_FALSE(Value.isString());
   EXPECT_FALSE(Value.isMatcher());
Index: unittests/ASTMatchers/Dynamic/ParserTest.cpp
===
--- unittests/ASTMatchers/Dynamic/ParserTest.cpp
+++ unittests/ASTMatchers/Dynamic/ParserTest.cpp
@@ -84,6 +84,21 @@
   EXPECT_EQ(false, Sema.Values[1].getBoolean());
 }
 
+TEST(ParserTest, ParseDouble) {
+  MockSema Sema;
+  Sema.parse("1.0");
+  Sema.parse("2.0f");
+  Sema.parse("34.56e-78");
+  Sema.parse("4.E+6");
+  Sema.parse("1");
+  EXPECT_EQ(5U, Sema.Values.size());
+  EXPECT_EQ(1.0, Sema.Values[0].getDouble());
+  EXPECT_EQ("1:1: Error parsing numeric literal: <2.0f>", Sema.Errors[1]);
+  EXPECT_EQ(34.56e-78, Sema.Values[2].getDouble());
+  EXPECT_EQ(4e+6, Sema.Values[3].getDouble());
+  EXPECT_FALSE(Sema.Values[4].isDouble());
+}
+
 TEST(ParserTest, ParseUnsigned) {
   MockSema Sema;
   Sema.parse("0");
@@ -95,8 +110,8 @@
   EXPECT_EQ(0U, Sema.Values[0].getUnsigned());
   EXPECT_EQ(123U, Sema.Values[1].getUnsigned());
   EXPECT_EQ(31U, Sema.Values[2].getUnsigned());
-  EXPECT_EQ("1:1: Error parsing unsigned token: <12345678901>", Sema.Errors[3]);
-  EXPECT_EQ("1:1: Error parsing unsigned token: <1a1>", Sema.Errors[4]);
+  EXPECT_EQ("1:1: Error parsing numeric literal: <12345678901>", Sema.Errors[3]);
+  EXPECT_EQ("1:1: Error parsing numeric literal: <1a1>", Sema.Errors[4]);
 }
 
 TEST(ParserTest, ParseString) {
Index: lib/ASTMatchers/Dynamic/VariantValue.cpp
===
--- lib/ASTMatchers/Dynamic/VariantValue.cpp
+++ lib/ASTMatchers/Dynamic/VariantValue.cpp
@@ -26,6 +26,8 @@
 return (Twine("Matcher<") + MatcherKind.asStringRef() + ">").str();
   case AK_Boolean:
 return "boolean";
+  case AK_Double:
+return "double";
   case AK_Unsigned:
 return "unsigned";
   case AK_String:
@@ -253,6 +255,10 @@
   setBoolean(Boolean);
 }
 
+VariantValue::VariantValue(double Double) : Type(VT_Nothing) {
+  setDouble(Double);
+}
+
 VariantValue::VariantValue(unsigned Unsigned) : Type(VT_Nothing) {
   setUnsigned(Unsigned);
 }
@@ -274,6 +280,9 @@
   case VT_Boolean:
 setBoolean(Other.getBoolean());
 break;
+  case VT_Double:
+setDouble(Other.getDouble());
+break;
   case VT_Unsigned:
 setUnsigned(Other.getUnsigned());
 break;
@@ -300,6 +309,7 @@
 break;
   // Cases that do nothing.
   case VT_Boolean:
+  case VT_Double:
   case VT_Unsigned:
   case VT_Nothing:
 break;
@@ -322,6 +332,21 @@
   Value.Boolean = NewValue;
 }
 
+bool VariantValue::isDouble() const {
+  return Type == VT_Double;
+}
+
+double VariantValue::getDouble() const {
+  assert(isDouble());
+  return Value.Double;
+}
+
+void VariantValue::setDouble(double 

[PATCH] D33093: [ASTMatchers] Add support for boolean literals

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn updated this revision to Diff 98781.
Lekensteyn edited the summary of this revision.
Lekensteyn added a comment.

v2: Minor updates (fixed comment: `//  ...` to `/// ").str();
+  case AK_Boolean:
+return "boolean";
   case AK_Unsigned:
 return "unsigned";
   case AK_String:
@@ -247,6 +249,10 @@
   *this = Other;
 }
 
+VariantValue::VariantValue(bool Boolean) : Type(VT_Nothing) {
+  setBoolean(Boolean);
+}
+
 VariantValue::VariantValue(unsigned Unsigned) : Type(VT_Nothing) {
   setUnsigned(Unsigned);
 }
@@ -265,6 +271,9 @@
   if (this == ) return *this;
   reset();
   switch (Other.Type) {
+  case VT_Boolean:
+setBoolean(Other.getBoolean());
+break;
   case VT_Unsigned:
 setUnsigned(Other.getUnsigned());
 break;
@@ -290,13 +299,29 @@
 delete Value.Matcher;
 break;
   // Cases that do nothing.
+  case VT_Boolean:
   case VT_Unsigned:
   case VT_Nothing:
 break;
   }
   Type = VT_Nothing;
 }
 
+bool VariantValue::isBoolean() const {
+  return Type == VT_Boolean;
+}
+
+bool VariantValue::getBoolean() const {
+  assert(isBoolean());
+  return Value.Boolean;
+}
+
+void VariantValue::setBoolean(bool NewValue) {
+  reset();
+  Type = VT_Boolean;
+  Value.Boolean = NewValue;
+}
+
 bool VariantValue::isUnsigned() const {
   return Type == VT_Unsigned;
 }
@@ -344,6 +369,12 @@
 
 bool VariantValue::isConvertibleTo(ArgKind Kind, unsigned *Specificity) const {
   switch (Kind.getArgKind()) {
+  case ArgKind::AK_Boolean:
+if (!isBoolean())
+  return false;
+*Specificity = 1;
+return true;
+
   case ArgKind::AK_Unsigned:
 if (!isUnsigned())
   return false;
@@ -383,6 +414,7 @@
   switch (Type) {
   case VT_String: return "String";
   case VT_Matcher: return getMatcher().getTypeAsString();
+  case VT_Boolean: return "Boolean";
   case VT_Unsigned: return "Unsigned";
   case VT_Nothing: return "Nothing";
   }
Index: lib/ASTMatchers/Dynamic/Parser.cpp
===
--- lib/ASTMatchers/Dynamic/Parser.cpp

[PATCH] D33094: [ASTMatchers] Add equals support for integer and boolean literals

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn added a comment.

In https://reviews.llvm.org/D33094#752153, @aaron.ballman wrote:

> Please be sure to regenerate the AST matcher documentation as well by running 
> clang/docs/tools/dump_ast_matchers.py


Will do, thanks for the hint.




Comment at: include/clang/ASTMatchers/ASTMatchers.h:3827
+/// \brief Instantiations for the \c equals matcher.
+/// TODO add support for FloatingLiteral and CharacterLiteral
+/// @{

aaron.ballman wrote:
> Why not add this support immediately rather than a TODO?
The parser would need additional changes for it to be usable in clang-query 
(see D33093 for boolean support), my initial focus was on supporting 
IntegerLiterals but bool was added to have an overload.

I'll look into adding support for other types.



Comment at: include/clang/ASTMatchers/ASTMatchersMacros.h:364
+/// comparing a \c ReturnType node against the a \c ParamType value.
+#define AST_CONCRETE_EQUALS_MATCHER(ReturnType, ParamType, OverloadId) 
\
+  inline ::clang::ast_matchers::internal::Matcher equals(  
\

aaron.ballman wrote:
> Instead of making the user of the macro pass in an overload id, could we make 
> use of the `__LINE__` macro to automate it? Given the length of the macro 
> name, I struggle to imagine many people accidentally defining two overloads 
> on the same line (and we can document this macro appropriately, of course).
Not sure how `__LINE__` would help, uniqueness is not the only requirement, it 
must also be a fixed name such that Registry can refer to it. (Uniqueness is 
also needed, otherwise it would be ambiguous).

Alternatively, the overload name can be removed, but then the returntype and 
paramtype for the marshaller should be explicitly specified. This would remove 
the magic numbers (for overload ID), is this an approach worth looking into?



Comment at: include/clang/ASTMatchers/ASTMatchersMacros.h:366
+  inline ::clang::ast_matchers::internal::Matcher equals(  
\
+  ParamType const ) {
\
+return ::clang::ast_matchers::internal::makeMatcher(   
\

aaron.ballman wrote:
> `const ParamType ` per the coding guidelines.
Will fix (this was copied from the above macros).


https://reviews.llvm.org/D33094



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33094: [ASTMatchers] Add equals support for integer and boolean literals

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn created this revision.

This allows the clang-query tool to use matchers like
"integerLiteral(equals(32))". The equals function is a template, so some
special trickery is needed.


https://reviews.llvm.org/D33094

Files:
  include/clang/ASTMatchers/ASTMatchers.h
  include/clang/ASTMatchers/ASTMatchersMacros.h
  lib/ASTMatchers/Dynamic/Registry.cpp
  unittests/ASTMatchers/Dynamic/RegistryTest.cpp


Index: unittests/ASTMatchers/Dynamic/RegistryTest.cpp
===
--- unittests/ASTMatchers/Dynamic/RegistryTest.cpp
+++ unittests/ASTMatchers/Dynamic/RegistryTest.cpp
@@ -511,6 +511,21 @@
   EXPECT_FALSE(matches("int i = 1;", Value));
 }
 
+TEST_F(RegistryTest, EqualsMatcher) {
+  Matcher IntegerStmt = constructMatcher(
+  "integerLiteral", constructMatcher("equals", VariantValue(42)))
+  .getTypedMatcher();
+  EXPECT_TRUE(matches("int x = 42;", IntegerStmt));
+  EXPECT_FALSE(matches("int x = 1;", IntegerStmt));
+
+  Matcher BooleanStmt = constructMatcher(
+  "cxxBoolLiteral", constructMatcher("equals", VariantValue(true)))
+  .getTypedMatcher();
+  EXPECT_TRUE(matches("bool x = true;", BooleanStmt));
+  EXPECT_FALSE(matches("bool x = false;", BooleanStmt));
+  EXPECT_FALSE(matches("bool x = 0;", BooleanStmt));
+}
+
 } // end anonymous namespace
 } // end namespace dynamic
 } // end namespace ast_matchers
Index: lib/ASTMatchers/Dynamic/Registry.cpp
===
--- lib/ASTMatchers/Dynamic/Registry.cpp
+++ lib/ASTMatchers/Dynamic/Registry.cpp
@@ -96,6 +96,9 @@
   REGISTER_OVERLOADED_2(references);
   REGISTER_OVERLOADED_2(thisPointerType);
 
+  // TODO add support for floats and char literals
+  REGISTER_OVERLOADED_2(equals);
+
   REGISTER_MATCHER(accessSpecDecl);
   REGISTER_MATCHER(addrLabelExpr);
   REGISTER_MATCHER(alignOfExpr);
Index: include/clang/ASTMatchers/ASTMatchersMacros.h
===
--- include/clang/ASTMatchers/ASTMatchersMacros.h
+++ include/clang/ASTMatchers/ASTMatchersMacros.h
@@ -359,6 +359,17 @@
   ::clang::ast_matchers::internal::BoundNodesTreeBuilder *Builder) 
\
   const
 
+/// \brief Defines functions and a type for the \c equals matcher, suitable for
+/// comparing a \c ReturnType node against the a \c ParamType value.
+#define AST_CONCRETE_EQUALS_MATCHER(ReturnType, ParamType, OverloadId) 
\
+  inline ::clang::ast_matchers::internal::Matcher equals(  
\
+  ParamType const ) {
\
+return ::clang::ast_matchers::internal::makeMatcher(   
\
+new internal::ValueEqualsMatcher(Param));   
\
+  }
\
+  typedef ::clang::ast_matchers::internal::Matcher(
\
+  _Type##OverloadId)(ParamType const )
+
 /// \brief Creates a variadic matcher for both a specific \c Type as well as
 /// the corresponding \c TypeLoc.
 #define AST_TYPE_MATCHER(NodeType, MatcherName)
\
Index: include/clang/ASTMatchers/ASTMatchers.h
===
--- include/clang/ASTMatchers/ASTMatchers.h
+++ include/clang/ASTMatchers/ASTMatchers.h
@@ -3813,7 +3813,7 @@
 ///   true
 /// \endcode
 ///
-/// Usable as: Matcher, Matcher,
+/// Usable as: Matcher, Matcher,
 ///Matcher, Matcher
 template 
 internal::PolymorphicMatcherWithParam1
@@ -3823,6 +3823,13 @@
 ValueT>(Value);
 }
 
+/// \brief Instantiations for the \c equals matcher.
+/// TODO add support for FloatingLiteral and CharacterLiteral
+/// @{
+AST_CONCRETE_EQUALS_MATCHER(CXXBoolLiteralExpr, bool, 0);
+AST_CONCRETE_EQUALS_MATCHER(IntegerLiteral, unsigned, 1);
+/// @}
+
 /// \brief Matches the operator Name of operator expressions (binary or
 /// unary).
 ///


Index: unittests/ASTMatchers/Dynamic/RegistryTest.cpp
===
--- unittests/ASTMatchers/Dynamic/RegistryTest.cpp
+++ unittests/ASTMatchers/Dynamic/RegistryTest.cpp
@@ -511,6 +511,21 @@
   EXPECT_FALSE(matches("int i = 1;", Value));
 }
 
+TEST_F(RegistryTest, EqualsMatcher) {
+  Matcher IntegerStmt = constructMatcher(
+  "integerLiteral", constructMatcher("equals", VariantValue(42)))
+  .getTypedMatcher();
+  EXPECT_TRUE(matches("int x = 42;", IntegerStmt));
+  EXPECT_FALSE(matches("int x = 1;", IntegerStmt));
+
+  Matcher BooleanStmt = constructMatcher(
+  "cxxBoolLiteral", constructMatcher("equals", VariantValue(true)))
+  .getTypedMatcher();
+  EXPECT_TRUE(matches("bool x = true;", BooleanStmt));
+  EXPECT_FALSE(matches("bool x = false;", BooleanStmt));
+  EXPECT_FALSE(matches("bool x = 0;", BooleanStmt));
+}
+
 } // end anonymous namespace
 } // end namespace dynamic
 } // end 

[PATCH] D33093: [ASTMatchers] Add support for boolean literals

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn created this revision.

Recognize boolean literals for future extensions ("equals(true)").
Note that a specific VariantValue constructor is added to resolve

ambiguity (like "Value = 5") between unsigned and bool.
---

Passes clang-test, patch for equals will follow shortly.


https://reviews.llvm.org/D33093

Files:
  include/clang/ASTMatchers/Dynamic/Parser.h
  include/clang/ASTMatchers/Dynamic/VariantValue.h
  lib/ASTMatchers/Dynamic/Marshallers.h
  lib/ASTMatchers/Dynamic/Parser.cpp
  lib/ASTMatchers/Dynamic/VariantValue.cpp
  unittests/ASTMatchers/Dynamic/ParserTest.cpp
  unittests/ASTMatchers/Dynamic/VariantValueTest.cpp

Index: unittests/ASTMatchers/Dynamic/VariantValueTest.cpp
===
--- unittests/ASTMatchers/Dynamic/VariantValueTest.cpp
+++ unittests/ASTMatchers/Dynamic/VariantValueTest.cpp
@@ -75,12 +75,14 @@
   EXPECT_TRUE(Value.isString());
   EXPECT_EQ("A", Value.getString());
   EXPECT_TRUE(Value.hasValue());
+  EXPECT_FALSE(Value.isBoolean());
   EXPECT_FALSE(Value.isUnsigned());
   EXPECT_FALSE(Value.isMatcher());
   EXPECT_EQ("String", Value.getTypeAsString());
 
   Value = VariantMatcher::SingleMatcher(recordDecl());
   EXPECT_TRUE(Value.hasValue());
+  EXPECT_FALSE(Value.isBoolean());
   EXPECT_FALSE(Value.isUnsigned());
   EXPECT_FALSE(Value.isString());
   EXPECT_TRUE(Value.isMatcher());
@@ -91,12 +93,22 @@
   Value = 17;
   EXPECT_TRUE(Value.isUnsigned());
   EXPECT_EQ(17U, Value.getUnsigned());
+  EXPECT_FALSE(Value.isBoolean());
   EXPECT_TRUE(Value.hasValue());
   EXPECT_FALSE(Value.isMatcher());
   EXPECT_FALSE(Value.isString());
 
+  Value = true;
+  EXPECT_TRUE(Value.isBoolean());
+  EXPECT_EQ(true, Value.getBoolean());
+  EXPECT_TRUE(Value.hasValue());
+  EXPECT_FALSE(Value.isUnsigned());
+  EXPECT_FALSE(Value.isMatcher());
+  EXPECT_FALSE(Value.isString());
+
   Value = VariantValue();
   EXPECT_FALSE(Value.hasValue());
+  EXPECT_FALSE(Value.isBoolean());
   EXPECT_FALSE(Value.isUnsigned());
   EXPECT_FALSE(Value.isString());
   EXPECT_FALSE(Value.isMatcher());
Index: unittests/ASTMatchers/Dynamic/ParserTest.cpp
===
--- unittests/ASTMatchers/Dynamic/ParserTest.cpp
+++ unittests/ASTMatchers/Dynamic/ParserTest.cpp
@@ -75,6 +75,15 @@
   ExpectedMatchersTy ExpectedMatchers;
 };
 
+TEST(ParserTest, ParseBoolean) {
+  MockSema Sema;
+  Sema.parse("true");
+  Sema.parse("false");
+  EXPECT_EQ(2U, Sema.Values.size());
+  EXPECT_EQ(true, Sema.Values[0].getBoolean());
+  EXPECT_EQ(false, Sema.Values[1].getBoolean());
+}
+
 TEST(ParserTest, ParseUnsigned) {
   MockSema Sema;
   Sema.parse("0");
Index: lib/ASTMatchers/Dynamic/VariantValue.cpp
===
--- lib/ASTMatchers/Dynamic/VariantValue.cpp
+++ lib/ASTMatchers/Dynamic/VariantValue.cpp
@@ -24,6 +24,8 @@
   switch (getArgKind()) {
   case AK_Matcher:
 return (Twine("Matcher<") + MatcherKind.asStringRef() + ">").str();
+  case AK_Boolean:
+return "boolean";
   case AK_Unsigned:
 return "unsigned";
   case AK_String:
@@ -247,6 +249,10 @@
   *this = Other;
 }
 
+VariantValue::VariantValue(bool Boolean) : Type(VT_Nothing) {
+  setBoolean(Boolean);
+}
+
 VariantValue::VariantValue(unsigned Unsigned) : Type(VT_Nothing) {
   setUnsigned(Unsigned);
 }
@@ -265,6 +271,9 @@
   if (this == ) return *this;
   reset();
   switch (Other.Type) {
+  case VT_Boolean:
+setBoolean(Other.getBoolean());
+break;
   case VT_Unsigned:
 setUnsigned(Other.getUnsigned());
 break;
@@ -290,13 +299,29 @@
 delete Value.Matcher;
 break;
   // Cases that do nothing.
+  case VT_Boolean:
   case VT_Unsigned:
   case VT_Nothing:
 break;
   }
   Type = VT_Nothing;
 }
 
+bool VariantValue::isBoolean() const {
+  return Type == VT_Boolean;
+}
+
+bool VariantValue::getBoolean() const {
+  assert(isBoolean());
+  return Value.Boolean;
+}
+
+void VariantValue::setBoolean(bool NewValue) {
+  reset();
+  Type = VT_Boolean;
+  Value.Boolean = NewValue;
+}
+
 bool VariantValue::isUnsigned() const {
   return Type == VT_Unsigned;
 }
@@ -344,6 +369,12 @@
 
 bool VariantValue::isConvertibleTo(ArgKind Kind, unsigned *Specificity) const {
   switch (Kind.getArgKind()) {
+  case ArgKind::AK_Boolean:
+if (!isBoolean())
+  return false;
+*Specificity = 1;
+return true;
+
   case ArgKind::AK_Unsigned:
 if (!isUnsigned())
   return false;
@@ -383,6 +414,7 @@
   switch (Type) {
   case VT_String: return "String";
   case VT_Matcher: return getMatcher().getTypeAsString();
+  case VT_Boolean: return "Boolean";
   case VT_Unsigned: return "Unsigned";
   case VT_Nothing: return "Nothing";
   }
Index: lib/ASTMatchers/Dynamic/Parser.cpp
===
--- lib/ASTMatchers/Dynamic/Parser.cpp
+++ lib/ASTMatchers/Dynamic/Parser.cpp
@@ -153,8 

[PATCH] D29827: [AVR] Add -mmcu option to the driver

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn updated this revision to Diff 95707.
Lekensteyn added a comment.

v2: rebase on commit ad25f8b712f1ef99020fcb7b5e31dd95b39c6112 (trunk@300661)

Based on the context, the change from lib/Driver/Tools.cpp -> 
lib/Driver/ToolChains/CommonArgs.cpp seems most appropriate (Gnu.cpp seems to 
be about mapping LLVM options to GCC?)


https://reviews.llvm.org/D29827

Files:
  include/clang/Driver/Options.td
  lib/Driver/ToolChains/CommonArgs.cpp
  test/Driver/avr-mmcu.c


Index: test/Driver/avr-mmcu.c
===
--- /dev/null
+++ test/Driver/avr-mmcu.c
@@ -0,0 +1,5 @@
+// A test for the propagation of the -mmcu option to -cc1 and -cc1as
+
+// RUN: %clang -### -target avr -mmcu=atmega328p -save-temps %s 2>&1 | 
FileCheck %s
+// CHECK: clang{{.*}} "-cc1" {{.*}} "-target-cpu" "atmega328p"
+// CHECK: clang{{.*}} "-cc1as" {{.*}} "-target-cpu" "atmega328p"
Index: lib/Driver/ToolChains/CommonArgs.cpp
===
--- lib/Driver/ToolChains/CommonArgs.cpp
+++ lib/Driver/ToolChains/CommonArgs.cpp
@@ -261,6 +261,12 @@
 arm::getARMArchCPUFromArgs(Args, MArch, MCPU, FromAs);
 return arm::getARMTargetCPU(MCPU, MArch, T);
   }
+
+  case llvm::Triple::avr:
+if (const Arg *A = Args.getLastArg(options::OPT_mmcu_EQ))
+  return A->getValue();
+return "";
+
   case llvm::Triple::mips:
   case llvm::Triple::mipsel:
   case llvm::Triple::mips64:
Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -1655,6 +1655,7 @@
 def municode : Joined<["-"], "municode">, Group, 
Flags<[DriverOption]>;
 def mthreads : Joined<["-"], "mthreads">, Group, 
Flags<[DriverOption]>;
 def mcpu_EQ : Joined<["-"], "mcpu=">, Group;
+def mmcu_EQ : Joined<["-"], "mmcu=">, Group;
 def mdynamic_no_pic : Joined<["-"], "mdynamic-no-pic">, Group;
 def mfix_and_continue : Flag<["-"], "mfix-and-continue">, 
Group;
 def mieee_fp : Flag<["-"], "mieee-fp">, Group;


Index: test/Driver/avr-mmcu.c
===
--- /dev/null
+++ test/Driver/avr-mmcu.c
@@ -0,0 +1,5 @@
+// A test for the propagation of the -mmcu option to -cc1 and -cc1as
+
+// RUN: %clang -### -target avr -mmcu=atmega328p -save-temps %s 2>&1 | FileCheck %s
+// CHECK: clang{{.*}} "-cc1" {{.*}} "-target-cpu" "atmega328p"
+// CHECK: clang{{.*}} "-cc1as" {{.*}} "-target-cpu" "atmega328p"
Index: lib/Driver/ToolChains/CommonArgs.cpp
===
--- lib/Driver/ToolChains/CommonArgs.cpp
+++ lib/Driver/ToolChains/CommonArgs.cpp
@@ -261,6 +261,12 @@
 arm::getARMArchCPUFromArgs(Args, MArch, MCPU, FromAs);
 return arm::getARMTargetCPU(MCPU, MArch, T);
   }
+
+  case llvm::Triple::avr:
+if (const Arg *A = Args.getLastArg(options::OPT_mmcu_EQ))
+  return A->getValue();
+return "";
+
   case llvm::Triple::mips:
   case llvm::Triple::mipsel:
   case llvm::Triple::mips64:
Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -1655,6 +1655,7 @@
 def municode : Joined<["-"], "municode">, Group, Flags<[DriverOption]>;
 def mthreads : Joined<["-"], "mthreads">, Group, Flags<[DriverOption]>;
 def mcpu_EQ : Joined<["-"], "mcpu=">, Group;
+def mmcu_EQ : Joined<["-"], "mmcu=">, Group;
 def mdynamic_no_pic : Joined<["-"], "mdynamic-no-pic">, Group;
 def mfix_and_continue : Flag<["-"], "mfix-and-continue">, Group;
 def mieee_fp : Flag<["-"], "mieee-fp">, Group;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29817: [AVR] Fix __AVR_xxx macro definitions

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn added a comment.

In https://reviews.llvm.org/D29817#674306, @dylanmckay wrote:

> Do you have commit access @Lekensteyn?


No I do not, please push it for me :-)


https://reviews.llvm.org/D29817



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29827: [AVR] Add -mmcu option to the driver

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn added inline comments.



Comment at: include/clang/Driver/Options.td:1613
 def mcpu_EQ : Joined<["-"], "mcpu=">, Group;
+def mmcu_EQ : Joined<["-"], "mmcu=">, Group;
 def mdynamic_no_pic : Joined<["-"], "mdynamic-no-pic">, Group;

dylanmckay wrote:
> Lekensteyn wrote:
> > jroelofs wrote:
> > > Would it make sense to have mcu be an alias for mcpu instead?
> > That would deviate from the GCC interface, so I have chosen for the current 
> > situation:
> > ```
> > $ avr-gcc -mmcu=avr2 -o /dev/null x.c
> > $ avr-gcc -mcpu=avr2 -o /dev/null x.c
> > avr-gcc: error: unrecognized command line option '-mcpu=avr2'
> > $ avr-gcc -march=avr2 -o /dev/null x.c
> > avr-gcc: error: unrecognized command line option '-march=avr2'
> > $ avr-gcc -v
> > ...
> > gcc version 6.3.0 (GCC)
> > ```
> I think @jroelofs  means that it is possible to make `mmcu` an alias of 
> `mmcu` internally. This would mean we wouldn't need to add AVR-specific 
> `getCPUName` handling.
If mmcu is made an alias of mcpu, wouldn't that mean that both `-mcpu` and 
`-mmcu` would be accepted by driver (undesirable)?
As far as I can see, `-target-cpu` must be passed to the frontend and 
assembler, `-mcpu=` is not recognized as option. And ensuring that `getCPUName` 
returns a non-empty string ensures that `-target-cpu` is passed.

I am quite new to the internals, so please let me know if I misunderstood 
something :-)


https://reviews.llvm.org/D29827



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29817: [AVR] Fix __AVR_xxx macro definitions

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn updated this revision to Diff 88025.
Lekensteyn marked an inline comment as done.
Lekensteyn added a comment.

Good catch! I just checked again and confirmed that there is only one line that 
had this issue (there are no other occurrences of `___"`).


https://reviews.llvm.org/D29817

Files:
  lib/Basic/Targets.cpp
  test/CodeGen/avr/target-cpu-defines/atmega328p.c
  test/CodeGen/avr/target-cpu-defines/attiny104.c

Index: test/CodeGen/avr/target-cpu-defines/attiny104.c
===
--- test/CodeGen/avr/target-cpu-defines/attiny104.c
+++ test/CodeGen/avr/target-cpu-defines/attiny104.c
@@ -3,5 +3,5 @@
 
 // CHECK: #define AVR 1
 // CHECK: #define __AVR 1
-// CHECK: #define __AVR_ATtiny104 1
+// CHECK: #define __AVR_ATtiny104__ 1
 // CHECK: #define __AVR__ 1
Index: test/CodeGen/avr/target-cpu-defines/atmega328p.c
===
--- test/CodeGen/avr/target-cpu-defines/atmega328p.c
+++ test/CodeGen/avr/target-cpu-defines/atmega328p.c
@@ -3,5 +3,5 @@
 
 // CHECK: #define AVR 1
 // CHECK: #define __AVR 1
-// CHECK: #define __AVR_ATmega328P 1
+// CHECK: #define __AVR_ATmega328P__ 1
 // CHECK: #define __AVR__ 1
Index: lib/Basic/Targets.cpp
===
--- lib/Basic/Targets.cpp
+++ lib/Basic/Targets.cpp
@@ -8462,244 +8462,244 @@
 // This list should be kept up-to-date with AVRDevices.td in LLVM.
 static ArrayRef AVRMcus = {
   { "at90s1200", "__AVR_AT90S1200__" },
-  { "attiny11", "__AVR_ATtiny11" },
-  { "attiny12", "__AVR_ATtiny12" },
-  { "attiny15", "__AVR_ATtiny15" },
-  { "attiny28", "__AVR_ATtiny28" },
-  { "at90s2313", "__AVR_AT90S2313" },
-  { "at90s2323", "__AVR_AT90S2323" },
-  { "at90s2333", "__AVR_AT90S2333" },
-  { "at90s2343", "__AVR_AT90S2343" },
-  { "attiny22", "__AVR_ATtiny22" },
-  { "attiny26", "__AVR_ATtiny26" },
-  { "at86rf401", "__AVR_AT86RF401" },
-  { "at90s4414", "__AVR_AT90S4414" },
-  { "at90s4433", "__AVR_AT90S4433" },
-  { "at90s4434", "__AVR_AT90S4434" },
-  { "at90s8515", "__AVR_AT90S8515" },
-  { "at90c8534", "__AVR_AT90c8534" },
-  { "at90s8535", "__AVR_AT90S8535" },
-  { "ata5272", "__AVR_ATA5272" },
-  { "attiny13", "__AVR_ATtiny13" },
-  { "attiny13a", "__AVR_ATtiny13A" },
-  { "attiny2313", "__AVR_ATtiny2313" },
-  { "attiny2313a", "__AVR_ATtiny2313A" },
-  { "attiny24", "__AVR_ATtiny24" },
-  { "attiny24a", "__AVR_ATtiny24A" },
-  { "attiny4313", "__AVR_ATtiny4313" },
-  { "attiny44", "__AVR_ATtiny44" },
-  { "attiny44a", "__AVR_ATtiny44A" },
-  { "attiny84", "__AVR_ATtiny84" },
-  { "attiny84a", "__AVR_ATtiny84A" },
-  { "attiny25", "__AVR_ATtiny25" },
-  { "attiny45", "__AVR_ATtiny45" },
-  { "attiny85", "__AVR_ATtiny85" },
-  { "attiny261", "__AVR_ATtiny261" },
-  { "attiny261a", "__AVR_ATtiny261A" },
-  { "attiny461", "__AVR_ATtiny461" },
-  { "attiny461a", "__AVR_ATtiny461A" },
-  { "attiny861", "__AVR_ATtiny861" },
-  { "attiny861a", "__AVR_ATtiny861A" },
-  { "attiny87", "__AVR_ATtiny87" },
-  { "attiny43u", "__AVR_ATtiny43U" },
-  { "attiny48", "__AVR_ATtiny48" },
-  { "attiny88", "__AVR_ATtiny88" },
-  { "attiny828", "__AVR_ATtiny828" },
-  { "at43usb355", "__AVR_AT43USB355" },
-  { "at76c711", "__AVR_AT76C711" },
-  { "atmega103", "__AVR_ATmega103" },
-  { "at43usb320", "__AVR_AT43USB320" },
-  { "attiny167", "__AVR_ATtiny167" },
-  { "at90usb82", "__AVR_AT90USB82" },
-  { "at90usb162", "__AVR_AT90USB162" },
-  { "ata5505", "__AVR_ATA5505" },
-  { "atmega8u2", "__AVR_ATmega8U2" },
-  { "atmega16u2", "__AVR_ATmega16U2" },
-  { "atmega32u2", "__AVR_ATmega32U2" },
-  { "attiny1634", "__AVR_ATtiny1634" },
-  { "atmega8", "__AVR_ATmega8" },
-  { "ata6289", "__AVR_ATA6289" },
-  { "atmega8a", "__AVR_ATmega8A" },
-  { "ata6285", "__AVR_ATA6285" },
-  { "ata6286", "__AVR_ATA6286" },
-  { "atmega48", "__AVR_ATmega48" },
-  { "atmega48a", "__AVR_ATmega48A" },
-  { "atmega48pa", "__AVR_ATmega48PA" },
-  { "atmega48p", "__AVR_ATmega48P" },
-  { "atmega88", "__AVR_ATmega88" },
-  { "atmega88a", "__AVR_ATmega88A" },
-  { "atmega88p", "__AVR_ATmega88P" },
-  { "atmega88pa", "__AVR_ATmega88PA" },
-  { "atmega8515", "__AVR_ATmega8515" },
-  { "atmega8535", "__AVR_ATmega8535" },
-  { "atmega8hva", "__AVR_ATmega8HVA" },
-  { "at90pwm1", "__AVR_AT90PWM1" },
-  { "at90pwm2", "__AVR_AT90PWM2" },
-  { "at90pwm2b", "__AVR_AT90PWM2B" },
-  { "at90pwm3", "__AVR_AT90PWM3" },
-  { "at90pwm3b", "__AVR_AT90PWM3B" },
-  { "at90pwm81", "__AVR_AT90PWM81" },
-  { "ata5790", "__AVR_ATA5790" },
-  { "ata5795", "__AVR_ATA5795" },
-  { "atmega16", "__AVR_ATmega16" },
-  { "atmega16a", "__AVR_ATmega16A" },
-  { "atmega161", "__AVR_ATmega161" },
-  { "atmega162", "__AVR_ATmega162" },
-  { "atmega163", "__AVR_ATmega163" },
-  { "atmega164a", "__AVR_ATmega164A" },
-  { "atmega164p", "__AVR_ATmega164P" },
-  { "atmega164pa", "__AVR_ATmega164PA" },
-  { "atmega165", "__AVR_ATmega165" },
-  { "atmega165a", "__AVR_ATmega165A" },
-  { 

[PATCH] D29827: [AVR] Add -mmcu option to the driver

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn added inline comments.



Comment at: include/clang/Driver/Options.td:1613
 def mcpu_EQ : Joined<["-"], "mcpu=">, Group;
+def mmcu_EQ : Joined<["-"], "mmcu=">, Group;
 def mdynamic_no_pic : Joined<["-"], "mdynamic-no-pic">, Group;

jroelofs wrote:
> Would it make sense to have mcu be an alias for mcpu instead?
That would deviate from the GCC interface, so I have chosen for the current 
situation:
```
$ avr-gcc -mmcu=avr2 -o /dev/null x.c
$ avr-gcc -mcpu=avr2 -o /dev/null x.c
avr-gcc: error: unrecognized command line option '-mcpu=avr2'
$ avr-gcc -march=avr2 -o /dev/null x.c
avr-gcc: error: unrecognized command line option '-march=avr2'
$ avr-gcc -v
...
gcc version 6.3.0 (GCC)
```


https://reviews.llvm.org/D29827



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29827: [AVR] Add -mmcu option to the driver

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn created this revision.

Add the AVR-specific -mmcu option for compatibility with GCC (GCC does not use
-mcpu nor -march for AVR). This option is needed to inform the frontend to
define some macros (for example) and the inform the assembler of the allowed
features, so add a test to check that.

Fixes PR#31569


https://reviews.llvm.org/D29827

Files:
  include/clang/Driver/Options.td
  lib/Driver/Tools.cpp
  test/Driver/avr-mmcu.c


Index: test/Driver/avr-mmcu.c
===
--- /dev/null
+++ test/Driver/avr-mmcu.c
@@ -0,0 +1,5 @@
+// A test for the propagation of the -mmcu option to -cc1 and -cc1as
+
+// RUN: %clang -### -target avr -mmcu=atmega328p -save-temps %s 2>&1 | 
FileCheck %s
+// CHECK: clang{{.*}} "-cc1" {{.*}} "-target-cpu" "atmega328p"
+// CHECK: clang{{.*}} "-cc1as" {{.*}} "-target-cpu" "atmega328p"
Index: lib/Driver/Tools.cpp
===
--- lib/Driver/Tools.cpp
+++ lib/Driver/Tools.cpp
@@ -2136,6 +2136,12 @@
 getARMArchCPUFromArgs(Args, MArch, MCPU, FromAs);
 return arm::getARMTargetCPU(MCPU, MArch, T);
   }
+
+  case llvm::Triple::avr:
+if (const Arg *A = Args.getLastArg(options::OPT_mmcu_EQ))
+  return A->getValue();
+return "";
+
   case llvm::Triple::mips:
   case llvm::Triple::mipsel:
   case llvm::Triple::mips64:
Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -1610,6 +1610,7 @@
 def municode : Joined<["-"], "municode">, Group, 
Flags<[DriverOption]>;
 def mthreads : Joined<["-"], "mthreads">, Group, 
Flags<[DriverOption]>;
 def mcpu_EQ : Joined<["-"], "mcpu=">, Group;
+def mmcu_EQ : Joined<["-"], "mmcu=">, Group;
 def mdynamic_no_pic : Joined<["-"], "mdynamic-no-pic">, Group;
 def mfix_and_continue : Flag<["-"], "mfix-and-continue">, 
Group;
 def mieee_fp : Flag<["-"], "mieee-fp">, Group;


Index: test/Driver/avr-mmcu.c
===
--- /dev/null
+++ test/Driver/avr-mmcu.c
@@ -0,0 +1,5 @@
+// A test for the propagation of the -mmcu option to -cc1 and -cc1as
+
+// RUN: %clang -### -target avr -mmcu=atmega328p -save-temps %s 2>&1 | FileCheck %s
+// CHECK: clang{{.*}} "-cc1" {{.*}} "-target-cpu" "atmega328p"
+// CHECK: clang{{.*}} "-cc1as" {{.*}} "-target-cpu" "atmega328p"
Index: lib/Driver/Tools.cpp
===
--- lib/Driver/Tools.cpp
+++ lib/Driver/Tools.cpp
@@ -2136,6 +2136,12 @@
 getARMArchCPUFromArgs(Args, MArch, MCPU, FromAs);
 return arm::getARMTargetCPU(MCPU, MArch, T);
   }
+
+  case llvm::Triple::avr:
+if (const Arg *A = Args.getLastArg(options::OPT_mmcu_EQ))
+  return A->getValue();
+return "";
+
   case llvm::Triple::mips:
   case llvm::Triple::mipsel:
   case llvm::Triple::mips64:
Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -1610,6 +1610,7 @@
 def municode : Joined<["-"], "municode">, Group, Flags<[DriverOption]>;
 def mthreads : Joined<["-"], "mthreads">, Group, Flags<[DriverOption]>;
 def mcpu_EQ : Joined<["-"], "mcpu=">, Group;
+def mmcu_EQ : Joined<["-"], "mmcu=">, Group;
 def mdynamic_no_pic : Joined<["-"], "mdynamic-no-pic">, Group;
 def mfix_and_continue : Flag<["-"], "mfix-and-continue">, Group;
 def mieee_fp : Flag<["-"], "mieee-fp">, Group;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29817: [AVR] Fix __AVR_xxx macro definitions

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn updated this revision to Diff 87986.
Lekensteyn added a comment.

Oops, missed the avrtiny patch. After updating that, it now actually passes the 
clang-test tests.


https://reviews.llvm.org/D29817

Files:
  lib/Basic/Targets.cpp
  test/CodeGen/avr/target-cpu-defines/atmega328p.c
  test/CodeGen/avr/target-cpu-defines/attiny104.c

Index: test/CodeGen/avr/target-cpu-defines/attiny104.c
===
--- test/CodeGen/avr/target-cpu-defines/attiny104.c
+++ test/CodeGen/avr/target-cpu-defines/attiny104.c
@@ -3,5 +3,5 @@
 
 // CHECK: #define AVR 1
 // CHECK: #define __AVR 1
-// CHECK: #define __AVR_ATtiny104 1
+// CHECK: #define __AVR_ATtiny104__ 1
 // CHECK: #define __AVR__ 1
Index: test/CodeGen/avr/target-cpu-defines/atmega328p.c
===
--- test/CodeGen/avr/target-cpu-defines/atmega328p.c
+++ test/CodeGen/avr/target-cpu-defines/atmega328p.c
@@ -3,5 +3,5 @@
 
 // CHECK: #define AVR 1
 // CHECK: #define __AVR 1
-// CHECK: #define __AVR_ATmega328P 1
+// CHECK: #define __AVR_ATmega328P__ 1
 // CHECK: #define __AVR__ 1
Index: lib/Basic/Targets.cpp
===
--- lib/Basic/Targets.cpp
+++ lib/Basic/Targets.cpp
@@ -8461,245 +8461,245 @@
 
 // This list should be kept up-to-date with AVRDevices.td in LLVM.
 static ArrayRef AVRMcus = {
-  { "at90s1200", "__AVR_AT90S1200__" },
-  { "attiny11", "__AVR_ATtiny11" },
-  { "attiny12", "__AVR_ATtiny12" },
-  { "attiny15", "__AVR_ATtiny15" },
-  { "attiny28", "__AVR_ATtiny28" },
-  { "at90s2313", "__AVR_AT90S2313" },
-  { "at90s2323", "__AVR_AT90S2323" },
-  { "at90s2333", "__AVR_AT90S2333" },
-  { "at90s2343", "__AVR_AT90S2343" },
-  { "attiny22", "__AVR_ATtiny22" },
-  { "attiny26", "__AVR_ATtiny26" },
-  { "at86rf401", "__AVR_AT86RF401" },
-  { "at90s4414", "__AVR_AT90S4414" },
-  { "at90s4433", "__AVR_AT90S4433" },
-  { "at90s4434", "__AVR_AT90S4434" },
-  { "at90s8515", "__AVR_AT90S8515" },
-  { "at90c8534", "__AVR_AT90c8534" },
-  { "at90s8535", "__AVR_AT90S8535" },
-  { "ata5272", "__AVR_ATA5272" },
-  { "attiny13", "__AVR_ATtiny13" },
-  { "attiny13a", "__AVR_ATtiny13A" },
-  { "attiny2313", "__AVR_ATtiny2313" },
-  { "attiny2313a", "__AVR_ATtiny2313A" },
-  { "attiny24", "__AVR_ATtiny24" },
-  { "attiny24a", "__AVR_ATtiny24A" },
-  { "attiny4313", "__AVR_ATtiny4313" },
-  { "attiny44", "__AVR_ATtiny44" },
-  { "attiny44a", "__AVR_ATtiny44A" },
-  { "attiny84", "__AVR_ATtiny84" },
-  { "attiny84a", "__AVR_ATtiny84A" },
-  { "attiny25", "__AVR_ATtiny25" },
-  { "attiny45", "__AVR_ATtiny45" },
-  { "attiny85", "__AVR_ATtiny85" },
-  { "attiny261", "__AVR_ATtiny261" },
-  { "attiny261a", "__AVR_ATtiny261A" },
-  { "attiny461", "__AVR_ATtiny461" },
-  { "attiny461a", "__AVR_ATtiny461A" },
-  { "attiny861", "__AVR_ATtiny861" },
-  { "attiny861a", "__AVR_ATtiny861A" },
-  { "attiny87", "__AVR_ATtiny87" },
-  { "attiny43u", "__AVR_ATtiny43U" },
-  { "attiny48", "__AVR_ATtiny48" },
-  { "attiny88", "__AVR_ATtiny88" },
-  { "attiny828", "__AVR_ATtiny828" },
-  { "at43usb355", "__AVR_AT43USB355" },
-  { "at76c711", "__AVR_AT76C711" },
-  { "atmega103", "__AVR_ATmega103" },
-  { "at43usb320", "__AVR_AT43USB320" },
-  { "attiny167", "__AVR_ATtiny167" },
-  { "at90usb82", "__AVR_AT90USB82" },
-  { "at90usb162", "__AVR_AT90USB162" },
-  { "ata5505", "__AVR_ATA5505" },
-  { "atmega8u2", "__AVR_ATmega8U2" },
-  { "atmega16u2", "__AVR_ATmega16U2" },
-  { "atmega32u2", "__AVR_ATmega32U2" },
-  { "attiny1634", "__AVR_ATtiny1634" },
-  { "atmega8", "__AVR_ATmega8" },
-  { "ata6289", "__AVR_ATA6289" },
-  { "atmega8a", "__AVR_ATmega8A" },
-  { "ata6285", "__AVR_ATA6285" },
-  { "ata6286", "__AVR_ATA6286" },
-  { "atmega48", "__AVR_ATmega48" },
-  { "atmega48a", "__AVR_ATmega48A" },
-  { "atmega48pa", "__AVR_ATmega48PA" },
-  { "atmega48p", "__AVR_ATmega48P" },
-  { "atmega88", "__AVR_ATmega88" },
-  { "atmega88a", "__AVR_ATmega88A" },
-  { "atmega88p", "__AVR_ATmega88P" },
-  { "atmega88pa", "__AVR_ATmega88PA" },
-  { "atmega8515", "__AVR_ATmega8515" },
-  { "atmega8535", "__AVR_ATmega8535" },
-  { "atmega8hva", "__AVR_ATmega8HVA" },
-  { "at90pwm1", "__AVR_AT90PWM1" },
-  { "at90pwm2", "__AVR_AT90PWM2" },
-  { "at90pwm2b", "__AVR_AT90PWM2B" },
-  { "at90pwm3", "__AVR_AT90PWM3" },
-  { "at90pwm3b", "__AVR_AT90PWM3B" },
-  { "at90pwm81", "__AVR_AT90PWM81" },
-  { "ata5790", "__AVR_ATA5790" },
-  { "ata5795", "__AVR_ATA5795" },
-  { "atmega16", "__AVR_ATmega16" },
-  { "atmega16a", "__AVR_ATmega16A" },
-  { "atmega161", "__AVR_ATmega161" },
-  { "atmega162", "__AVR_ATmega162" },
-  { "atmega163", "__AVR_ATmega163" },
-  { "atmega164a", "__AVR_ATmega164A" },
-  { "atmega164p", "__AVR_ATmega164P" },
-  { "atmega164pa", "__AVR_ATmega164PA" },
-  { "atmega165", "__AVR_ATmega165" },
-  { "atmega165a", "__AVR_ATmega165A" },
-  { "atmega165p", "__AVR_ATmega165P" },
-  { "atmega165pa", "__AVR_ATmega165PA" },
-  { 

[PATCH] D29817: [AVR} Fix __AVR_xxx macro definitions

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn added a comment.

Without this patch, compilation of a program using avr/io.h fails. Tested with:

  clang --target=avr -Xclang -target-cpu -Xclang atmega328p -I/usr/avr/include 
-Os led.c -c -o /dev/null

Btw, it seems that avr-libc 2.0.0 also uses `__AVR_ARCH__`, `__AVR_XMEGA__` and 
`__AVR_3_BYTE_PC__` in some places, but these are not defined in Clang.


https://reviews.llvm.org/D29817



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29817: [AVR} Fix __AVR_xxx macro definitions

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn created this revision.

The -mmcu option for GCC sets macros like __AVR_ATmega328P__ (with the trailing
underscores), be sure to include these underscores for Clangs -mcpu option.

See "AVR Built-in Macros" in https://gcc.gnu.org/onlinedocs/gcc/AVR-Options.html


https://reviews.llvm.org/D29817

Files:
  lib/Basic/Targets.cpp
  test/CodeGen/avr/target-cpu-defines/atmega328p.c

Index: test/CodeGen/avr/target-cpu-defines/atmega328p.c
===
--- test/CodeGen/avr/target-cpu-defines/atmega328p.c
+++ test/CodeGen/avr/target-cpu-defines/atmega328p.c
@@ -3,5 +3,5 @@
 
 // CHECK: #define AVR 1
 // CHECK: #define __AVR 1
-// CHECK: #define __AVR_ATmega328P 1
+// CHECK: #define __AVR_ATmega328P__ 1
 // CHECK: #define __AVR__ 1
Index: lib/Basic/Targets.cpp
===
--- lib/Basic/Targets.cpp
+++ lib/Basic/Targets.cpp
@@ -8461,245 +8461,245 @@
 
 // This list should be kept up-to-date with AVRDevices.td in LLVM.
 static ArrayRef AVRMcus = {
-  { "at90s1200", "__AVR_AT90S1200__" },
-  { "attiny11", "__AVR_ATtiny11" },
-  { "attiny12", "__AVR_ATtiny12" },
-  { "attiny15", "__AVR_ATtiny15" },
-  { "attiny28", "__AVR_ATtiny28" },
-  { "at90s2313", "__AVR_AT90S2313" },
-  { "at90s2323", "__AVR_AT90S2323" },
-  { "at90s2333", "__AVR_AT90S2333" },
-  { "at90s2343", "__AVR_AT90S2343" },
-  { "attiny22", "__AVR_ATtiny22" },
-  { "attiny26", "__AVR_ATtiny26" },
-  { "at86rf401", "__AVR_AT86RF401" },
-  { "at90s4414", "__AVR_AT90S4414" },
-  { "at90s4433", "__AVR_AT90S4433" },
-  { "at90s4434", "__AVR_AT90S4434" },
-  { "at90s8515", "__AVR_AT90S8515" },
-  { "at90c8534", "__AVR_AT90c8534" },
-  { "at90s8535", "__AVR_AT90S8535" },
-  { "ata5272", "__AVR_ATA5272" },
-  { "attiny13", "__AVR_ATtiny13" },
-  { "attiny13a", "__AVR_ATtiny13A" },
-  { "attiny2313", "__AVR_ATtiny2313" },
-  { "attiny2313a", "__AVR_ATtiny2313A" },
-  { "attiny24", "__AVR_ATtiny24" },
-  { "attiny24a", "__AVR_ATtiny24A" },
-  { "attiny4313", "__AVR_ATtiny4313" },
-  { "attiny44", "__AVR_ATtiny44" },
-  { "attiny44a", "__AVR_ATtiny44A" },
-  { "attiny84", "__AVR_ATtiny84" },
-  { "attiny84a", "__AVR_ATtiny84A" },
-  { "attiny25", "__AVR_ATtiny25" },
-  { "attiny45", "__AVR_ATtiny45" },
-  { "attiny85", "__AVR_ATtiny85" },
-  { "attiny261", "__AVR_ATtiny261" },
-  { "attiny261a", "__AVR_ATtiny261A" },
-  { "attiny461", "__AVR_ATtiny461" },
-  { "attiny461a", "__AVR_ATtiny461A" },
-  { "attiny861", "__AVR_ATtiny861" },
-  { "attiny861a", "__AVR_ATtiny861A" },
-  { "attiny87", "__AVR_ATtiny87" },
-  { "attiny43u", "__AVR_ATtiny43U" },
-  { "attiny48", "__AVR_ATtiny48" },
-  { "attiny88", "__AVR_ATtiny88" },
-  { "attiny828", "__AVR_ATtiny828" },
-  { "at43usb355", "__AVR_AT43USB355" },
-  { "at76c711", "__AVR_AT76C711" },
-  { "atmega103", "__AVR_ATmega103" },
-  { "at43usb320", "__AVR_AT43USB320" },
-  { "attiny167", "__AVR_ATtiny167" },
-  { "at90usb82", "__AVR_AT90USB82" },
-  { "at90usb162", "__AVR_AT90USB162" },
-  { "ata5505", "__AVR_ATA5505" },
-  { "atmega8u2", "__AVR_ATmega8U2" },
-  { "atmega16u2", "__AVR_ATmega16U2" },
-  { "atmega32u2", "__AVR_ATmega32U2" },
-  { "attiny1634", "__AVR_ATtiny1634" },
-  { "atmega8", "__AVR_ATmega8" },
-  { "ata6289", "__AVR_ATA6289" },
-  { "atmega8a", "__AVR_ATmega8A" },
-  { "ata6285", "__AVR_ATA6285" },
-  { "ata6286", "__AVR_ATA6286" },
-  { "atmega48", "__AVR_ATmega48" },
-  { "atmega48a", "__AVR_ATmega48A" },
-  { "atmega48pa", "__AVR_ATmega48PA" },
-  { "atmega48p", "__AVR_ATmega48P" },
-  { "atmega88", "__AVR_ATmega88" },
-  { "atmega88a", "__AVR_ATmega88A" },
-  { "atmega88p", "__AVR_ATmega88P" },
-  { "atmega88pa", "__AVR_ATmega88PA" },
-  { "atmega8515", "__AVR_ATmega8515" },
-  { "atmega8535", "__AVR_ATmega8535" },
-  { "atmega8hva", "__AVR_ATmega8HVA" },
-  { "at90pwm1", "__AVR_AT90PWM1" },
-  { "at90pwm2", "__AVR_AT90PWM2" },
-  { "at90pwm2b", "__AVR_AT90PWM2B" },
-  { "at90pwm3", "__AVR_AT90PWM3" },
-  { "at90pwm3b", "__AVR_AT90PWM3B" },
-  { "at90pwm81", "__AVR_AT90PWM81" },
-  { "ata5790", "__AVR_ATA5790" },
-  { "ata5795", "__AVR_ATA5795" },
-  { "atmega16", "__AVR_ATmega16" },
-  { "atmega16a", "__AVR_ATmega16A" },
-  { "atmega161", "__AVR_ATmega161" },
-  { "atmega162", "__AVR_ATmega162" },
-  { "atmega163", "__AVR_ATmega163" },
-  { "atmega164a", "__AVR_ATmega164A" },
-  { "atmega164p", "__AVR_ATmega164P" },
-  { "atmega164pa", "__AVR_ATmega164PA" },
-  { "atmega165", "__AVR_ATmega165" },
-  { "atmega165a", "__AVR_ATmega165A" },
-  { "atmega165p", "__AVR_ATmega165P" },
-  { "atmega165pa", "__AVR_ATmega165PA" },
-  { "atmega168", "__AVR_ATmega168" },
-  { "atmega168a", "__AVR_ATmega168A" },
-  { "atmega168p", "__AVR_ATmega168P" },
-  { "atmega168pa", "__AVR_ATmega168PA" },
-  { "atmega169", "__AVR_ATmega169" },
-  { "atmega169a", "__AVR_ATmega169A" },
-  { "atmega169p", "__AVR_ATmega169P" },
-  { "atmega169pa", "__AVR_ATmega169PA" },
-  { "atmega32", "__AVR_ATmega32" },