Try to unbreak Win build differently after 973519826edb76

[thakis]

Summary

Looks like the MS STL wants StringMapKeyIterator::operator*() to be const.
Return the result by copy instead of reference to do that.
Assigning to a hash map key iterator doesn't make sense anyways.

Also reverts 123f811fe5b0b which is now hopefully no longer needed.

[GMNGeoffrey]

I'm seeing a build failure coming out of this:

llvm-project/lld/wasm/Writer.cpp:521:16: error: non-const lvalue reference to type 'llvm::StringRef' cannot bind to a temporary of type 'llvm::StringRef'

for (auto &feature : used.keys()) {

Looks like pre-merge checks failed with the same error: https://buildkite.com/llvm-project/premerge-checks/builds/54930#07373971-3d37-49cf-9def-22c0d724ee23

[thakis]

In D109167#2980560, @GMNGeoffrey wrote:

I'm seeing a build failure coming out of this:

llvm-project/lld/wasm/Writer.cpp:521:16: error: non-const lvalue reference to type 'llvm::StringRef' cannot bind to a temporary of type 'llvm::StringRef'

for (auto &feature : used.keys()) {

Looks like pre-merge checks failed with the same error: https://buildkite.com/llvm-project/premerge-checks/builds/54930#07373971-3d37-49cf-9def-22c0d724ee23

I had fixed this in https://github.com/llvm/llvm-project/commit/9d227543890e721b95303430ee1427ce5aa7292f . Precommit bot on this change was green though as far as I can tell.

[GMNGeoffrey]

I had fixed this in https://github.com/llvm/llvm-project/commit/9d227543890e721b95303430ee1427ce5aa7292f . Precommit bot on this change was green though as far as I can tell.

Harbormaster comments with a green check mark that build has completed, but if you look at the top of the revision it has the build results. For some reason that I don't understand, the actual status at the top disappears as soon as the commit has landed, which is... very unhelpful. If you click on the Harbormaster comment now, you can see that they failed. I've reopened a diff with https://reviews.llvm.org/D109181 and will reland if premerge checks pass. Sorry that my revert went past the fix. AFAIU, I was following the guidelines in https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy. I think it would be nice to comment on the review that you are already preparing a fix (but I have no idea what the standard practice is). If I had known you were then I wouldn't have reverted.

[GMNGeoffrey]

In D109167#2980604, @GMNGeoffrey wrote:

I had fixed this in https://github.com/llvm/llvm-project/commit/9d227543890e721b95303430ee1427ce5aa7292f . Precommit bot on this change was green though as far as I can tell.

Harbormaster comments with a green check mark that build has completed, but if you look at the top of the revision it has the build results. For some reason that I don't understand, the actual status at the top disappears as soon as the commit has landed, which is... very unhelpful. If you click on the Harbormaster comment now, you can see that they failed. I've reopened a diff with https://reviews.llvm.org/D109181 and will reland if premerge checks pass. Sorry that my revert went past the fix. AFAIU, I was following the guidelines in https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy. I think it would be nice to comment on the review that you are already preparing a fix (but I have no idea what the standard practice is). If I had known you were then I wouldn't have reverted.

@goncharov @mehdi_amini is there a way to change the way the phab UI or the pre-merge checks work here to make failing pre-merge checks clearer? This isn't the first time I've seen someone be very confused about this UI

[thakis]

In D109167#2980604, @GMNGeoffrey wrote:

I had fixed this in https://github.com/llvm/llvm-project/commit/9d227543890e721b95303430ee1427ce5aa7292f . Precommit bot on this change was green though as far as I can tell.

Harbormaster comments with a green check mark that build has completed, but if you look at the top of the revision it has the build results.

Thanks, I didn't realize this. I thought this passed precommit, else I wouldn't have landed it.

Perfectly fine to revert things that break the build of course. Timing was a bit unfortunate, but reverting broken things is of course A-ok (especially with failing pre-merge testing). Thanks for relanding :)

For some reason that I don't understand, the actual status at the top disappears as soon as the commit has landed, which is... very unhelpful. If you click on the Harbormaster comment now, you can see that they failed. I've reopened a diff with https://reviews.llvm.org/D109181 and will reland if premerge checks pass. Sorry that my revert went past the fix. AFAIU, I was following the guidelines in https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy. I think it would be nice to comment on the review that you are already preparing a fix (but I have no idea what the standard practice is). If I had known you were then I wouldn't have reverted.

[mehdi_amini]

@goncharov @mehdi_amini is there a way to change the way the phab UI or the pre-merge checks work here to make failing pre-merge checks clearer? This isn't the first time I've seen someone be very confused about this UI

I asked the same thing a few days ago and @goncharov said something was cooking in this area!

clang/lib/Driver/ToolChains/Arch/X86.cpp

if (Triple.getArch() == llvm::Triple::x86) {
       ArchMap.insert({
           {"IA32", "i386"},
           {"SSE", "pentium3"},
           {"SSE2", "pentium4"},
       });
     }
     StringRef CPU = ArchMap.lookup(A->getValue());
     if (CPU.empty()) {
-      std::vector<StringRef> ValidArchs;
-      for (StringRef Key : ArchMap.keys())
-        ValidArchs.push_back(Key);
+      std::vector<StringRef> ValidArchs{ArchMap.keys().begin(),
+                                        ArchMap.keys().end()};
       sort(ValidArchs);
       D.Diag(diag::warn_drv_invalid_arch_name_with_suggestion)
           << A->getValue() << (Triple.getArch() == llvm::Triple::x86)
           << join(ValidArchs, ", ");
     }
     return std::string(CPU);
   }
 

llvm/include/llvm/ADT/StringMap.h

using base = iterator_adaptor_base<StringMapKeyIterator<ValueTy>,
                                      StringMapConstIterator<ValueTy>,
                                      std::forward_iterator_tag, StringRef>;
 
 public:
   StringMapKeyIterator() = default;
   explicit StringMapKeyIterator(StringMapConstIterator<ValueTy> Iter)
       : base(std::move(Iter)) {}
 
-  StringRef &operator*() {
-    Key = this->wrapped()->getKey();
-    return Key;
+  StringRef operator*() const {
+    return this->wrapped()->getKey();
   }
-
-private:
-  StringRef Key;
 };
 
 } // end namespace llvm
 
 #endif // LLVM_ADT_STRINGMAP_H

llvm/unittests/ADT/StringMapTest.cpp

TEST_F(StringMapTest, InsertOrAssignTest) {
   EXPECT_FALSE(try2.second);
   EXPECT_EQ(2, try2.first->second.v);
   EXPECT_EQ(2, try1.first->second.move);
 
   EXPECT_EQ(try1.first, try2.first);
   EXPECT_EQ(0, try1.first->second.copy);
 }
 
-TEST_F(StringMapTest, IterMapKeys) {
+TEST_F(StringMapTest, IterMapKeysVector) {
+  StringMap<int> Map;
+  Map["A"] = 1;
+  Map["B"] = 2;
+  Map["C"] = 3;
+  Map["D"] = 3;
+
+  std::vector<StringRef> Keys{Map.keys().begin(), Map.keys().end()};
+  llvm::sort(Keys);
+
+  std::vector<StringRef> Expected{{"A", "B", "C", "D"}};
+  EXPECT_EQ(Expected, Keys);
+}
+
+TEST_F(StringMapTest, IterMapKeysSmallVector) {
   StringMap<int> Map;
   Map["A"] = 1;
   Map["B"] = 2;
   Map["C"] = 3;
   Map["D"] = 3;
 
   auto Keys = to_vector<4>(Map.keys());
   llvm::sort(Keys);