folly/Foreach.h: allow FOR_EACH to be nested with no shadowing

Summary:
Prior to this change, any nested use of FOR_EACH would
induce a shadowed declaration for each of the two local
state variables it declares.

This makes the names of those variables __LINE__-dependent,
so that there is no shadowing, as long as each nested use
is on a different line.

This also adds a new test that (prior to this change) would fail to
compile with an option like -Werror=shadow-compatible-local.

Since this change relies on cpp token concatenation, I have included
The fix defines a new helper macro, _FE_ANON, to derive each new variable
name. I wondered whether to do this for every other FOR_* macro here, but
since so far, I have encountered more than 10 cases of nested FOR_EACH
uses in a large corpus, but no nesting of any other FOR_* macro, I am
content to do it only for this one.

Reviewed By: yfeldblum

Differential Revision: D3992956

fbshipit-source-id: f26fba89bc661bb9d22747dec0acdcf8c648fb83

diff --git a/folly/Foreach.h b/folly/Foreach.h
index d362c0bab2b3378bddb0637c11287c8a007838ca..84663ef363604da9fb23f059ed4c0565abe3cb0d 100644 (file)
--- a/folly/Foreach.h
+++ b/folly/Foreach.h
@@ -29,13 +29,13 @@
  * and everything is taken care of.
  *
  * The implementation is a bit convoluted to make sure the container is
- * only evaluated once (however, keep in mind that c.end() is evaluated
+ * evaluated only once (however, keep in mind that c.end() is evaluated
  * at every pass through the loop). To ensure the container is not
  * evaluated multiple times, the macro defines one do-nothing if
  * statement to inject the Boolean variable FOR_EACH_state1, and then a
  * for statement that is executed only once, which defines the variable
- * FOR_EACH_state2 holding a rvalue reference to the container being
- * iterated. The workhorse is the last loop, which uses the just defined
+ * FOR_EACH_state2 holding an rvalue reference to the container being
+ * iterated. The workhorse is the last loop, which uses the just-defined
  * rvalue reference FOR_EACH_state2.
  *
  * The state variables are nested so they don't interfere; you can use
@@ -47,18 +47,25 @@
  */
 
 #include <type_traits>
+#include <folly/Preprocessor.h>
+
+/*
+ * Form a local variable name from "FOR_EACH_" x __LINE__, so that
+ * FOR_EACH can be nested without creating shadowed declarations.
+ */
+#define _FE_ANON(x) FB_CONCATENATE(FOR_EACH_, FB_CONCATENATE(x, __LINE__))
 
 /*
  * Shorthand for:
  *   for (auto i = c.begin(); i != c.end(); ++i)
- * except that c is only evaluated once.
+ * except that c is evaluated only once.
  */
-#define FOR_EACH(i, c)                              \
-  if (bool FOR_EACH_state1 = false) {} else         \
-    for (auto && FOR_EACH_state2 = (c);             \
-         !FOR_EACH_state1; FOR_EACH_state1 = true)  \
-      for (auto i = FOR_EACH_state2.begin();        \
-           i != FOR_EACH_state2.end(); ++i)
+#define FOR_EACH(i, c)                                  \
+  if (bool _FE_ANON(s1_) = false) {} else               \
+    for (auto && _FE_ANON(s2_) = (c);                   \
+         !_FE_ANON(s1_); _FE_ANON(s1_) = true)          \
+      for (auto i = _FE_ANON(s2_).begin();              \
+           i != _FE_ANON(s2_).end(); ++i)
 
 /*
  * Similar to FOR_EACH, but iterates the container backwards by

diff --git a/folly/test/ForeachTest.cpp b/folly/test/ForeachTest.cpp
index 9904084d631ce816280e265559c0903a85179789..d7de3117296b4a004cc72adebd0e93ff004fd10f 100644 (file)
--- a/folly/test/ForeachTest.cpp
+++ b/folly/test/ForeachTest.cpp
@@ -40,6 +40,18 @@ TEST(Foreach, ForEachRvalue) {
   EXPECT_EQ(0, n);
 }
 
+TEST(Foreach, ForEachNested) {
+  const std::string hello = "hello";
+  size_t n = 0;
+  FOR_EACH(i, hello) {
+    FOR_EACH(j, hello) {
+      ++n;
+    }
+  }
+  auto len = hello.size();
+  EXPECT_EQ(len * len, n);
+}
+
 TEST(Foreach, ForEachKV) {
   std::map<std::string, int> testMap;
   testMap["abc"] = 1;