Get rid of #includes that do not spark joy #32080

varungandhi-apple · 2020-05-29T10:42:45Z

In case anyone is interested in how I did this, I basically ran the following script with the different headers:

# add all the headers here
declare -a HEADERLIST=("Optional" "StringRef" "ArrayRef")

# please don't judge my bash quality kthx
for HEADER in "${HEADERLIST[@]}"
do
    rg -c "$HEADER" --color never \
        | grep ':1$' \
        | sed -e 's/:1//' \
        | xargs rg --color never -e "include \"llvm/ADT/$HEADER.h\""
done

Try removing includes from each group one by one, fixing compiler errors as necessary.

This mostly works, except sometimes StringRef gets used for StringLiteral.

varungandhi-apple · 2020-05-29T10:43:00Z

@swift-ci please test

swift-ci · 2020-05-29T12:01:24Z

Build failed
Swift Test Linux Platform
Git Sha - a94a6bb92d210c586a7510ca0c7e0594b4ec5261

varungandhi-apple · 2020-05-29T15:53:48Z

@swift-ci please test Linux platform

compnerd · 2020-05-29T16:04:46Z

Could you please clang-format the patches? I noticed at least one where I expect clang-format will change the diff.

Im not sure I understand the change ... is this effectively attempting to run IWYU over the tree without running IWYU? (https://include-what-you-use.org/). If so, the IWYU might be better to use (as it is better known/understood) than "redundant headers" as I was expecting the headers to be listed twice in the file based on the name.

varungandhi-apple · 2020-05-29T16:16:22Z

Could you please clang-format the patches? I noticed at least one where I expect clang-format will change the diff.

That's surprising, I've formatted things based on the style we use today. I will try this out.

Im not sure I understand the change ... is this effectively attempting to run IWYU over the tree without running IWYU? (https://include-what-you-use.org/). If so, the IWYU might be better to use (as it is better known/understood) than "redundant headers" as I was expecting the headers to be listed twice in the file based on the name.

I said "redundant" because those includes are redundant in a way; they are not getting used in the file they are being included in. I don't know if there's a standard adjective for "include that is not being used directly in the same file the include is present in".

It just so happened that some files were not including what they were using, and that only showed up because I removed some includes higher up the chain, so I manually did an include-what-you-use to fix that.

I did not realize that IWYU also removed includes. I thought it only added includes based on the name and second-hand information I read in a StackOverflow answer (which I hit when I was searching for tools to do this). Now that I know that my initial impression was incorrect, I'm going to try to use it.

compnerd · 2020-05-29T16:44:31Z

Ah, okay, so it was an attempt to do the IWYU cleanup, that sounds great! I don't know if you have the time to do more here, but a bot would be great to have to ensure that we don't regress (hey, I can dream right? :-D)

BTW, if you have time, grabbing some numbers would be cool - I imagine that you are not just cleaning the code base, you are also reducing compile times (not necessary, just for feeding the number monster).

These exercises are often undervalued but they really do help the project.

varungandhi-apple · 2020-05-29T17:20:19Z

Ah, okay, so it was an attempt to do the IWYU cleanup, that sounds great!

Thanks. (Tangent: I think IWYU should be rebranded to IOWYU (include-only-what-you-use), because that also carries the nuance of don't-include-what-you-don't-use but that's a different matter.)

I don't know if you have the time to do more here, but a bot would be great to have to ensure that we don't regress (hey, I can dream right? :-D)

I think of this enterprise in baby steps.

Run the thing.
Measure the effect.
Add instructions to docs/ on how to reliably build and run IWYU. Make sure those are working on different supported development platforms.
Try to figure out how often IWYU master has regressions, and if it consistently builds with apple/llvm-project, since we wouldn't want people to be blocked on a broken testing dependency.
If the answer to 4. is optimistic, start a policy conversation on whether we can take a dependency on IWYU for testing.
If the answer to 5. is optimistic, put in the work to get the testing working with good developer ergonomics.

I don't want to prematurely sign myself up to do things on this list. You mentioned "grabbing numbers would be cool", yes -- I deliberately avoided mentioning that in the PR text because I didn't want people to hold me to it. 😛 This is all "bonus" work. I'll see what I can get done, 2 seems very much within reach, 3 also seems do-able-ish without too much time (famous last words etc.), but I don't want to make promises I can't keep.

varungandhi-apple · 2020-05-29T17:56:55Z

^ Adding to this, I immediately ran into to two issues trying to use IWYU.

IWYU master didn't build with apple/llvm-project's swift/master branch. Worked around this by using llvm-project's master.

It somehow ends up hitting a cycle error when building Clang, even though the code compiles fine without IWYU:

Error running '/Users/varun/tmp/build/bin/include-what-you-use': In file included from /Users/varun/foss-swift/llvm-project/clang/lib/AST/ASTContext.cpp:13:
In file included from /Users/varun/foss-swift/llvm-project/clang/include/clang/AST/ASTContext.h:17:
In file included from /Users/varun/foss-swift/llvm-project/clang/include/clang/AST/ASTContextAllocate.h:17:
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/cstddef:44:15: fatal error: 'stddef.h' file not found
#include_next <stddef.h>
              ^~~~~~~~~~
Cycle in include-mapping:
  "Interp/Context.h" ->
  "Interp/Context.h"

These are certainly solvable problems, it's just that I'm skeptical that it will be directly usable in the imminent future given such issues. E.g. if we're changing Xcodes and it hits spurious errors like this, that'd block people from getting work done.

compnerd · 2020-05-29T17:59:35Z

Oh, agreed, thats why I said that would be nice - I wouldn't say that they are needed for the cleanup that you are doing. But, I figured that Id push my luck and see if there is more good stuff the project could get out of your efforts 😄.

Personally, I would raise 3 over 2 - making it easier for others to do is more valuable than the measurements - it is improving the quality of the code, the benefits to build times are pure icing! If you drop 2 to get 4/5, I would be fine with that even.

I think that the one thing that would be nice is that you actually put that list into the commit message so that even if you don't get to it, someone else may be able to follow up. Or maybe there should be a "things that would be nice to do" list that is separate?

varungandhi-apple · 2020-05-29T18:04:22Z

Oh, agreed, thats why I said that would be nice - I wouldn't say that they are needed for the cleanup that you are doing.

Fair enough, I was overinterpreting what you said. My bad.

Personally, I would raise 3 over 2 - making it easier for others to do is more valuable than the measurements

Hmm. That's a good point. I was thinking that making it easier is valuable if there are tangible benefits. Otherwise, maybe people wouldn't want to use it, because there was no demonstrated benefit.

I think that the one thing that would be nice is that you actually put that list into the commit message so that even if you don't get to it, someone else may be able to follow up. Or maybe there should be a "things that would be nice to do" list that is separate?

I think it's easier to miss if it's in a commit message. I can add new documentation on this though with the list.

swift-ci · 2020-05-29T18:29:16Z

Build failed
Swift Test Linux Platform
Git Sha - a94a6bb92d210c586a7510ca0c7e0594b4ec5261

compnerd · 2020-05-29T19:11:16Z

I was thinking that making it easier is valuable if there are tangible benefits. Otherwise, maybe people wouldn't want to use it, because there was no demonstrated benefit.

Hmm, maybe I'm mistaken in this, but I thought it was generally pretty well accepted that reducing preprocessed content is beneficial. It reduces compile times, reduce file system overheads, reduces object size (intermediate build artifacts), and link times while also reducing unnecessary recompilation.

varungandhi-apple · 2020-05-29T19:25:31Z

Well, it kinda' depends on who is the target audience. I certainly didn't know about the impact until reading Bruce Dawson's blog posts related to compile time.

In any case, let's not debate the nitty-gritties, since we seem to mostly be in agreement. I am going to try to get IWYU to work. If I am able to get it working in the time that I have, I'll add some documentation on how to set it up and use it (in a separate PR, not this one).

If you have any specific suggestions on rewording the commit messages in this PR to not use the word "redundant", I'm happy to change the commit messages to make things clearer based on your suggestion.

compnerd · 2020-05-29T20:36:47Z

Yeah, I think that we are in agreement and are discussing hypotheticals. I think that just referencing the operation as "include what you use" is a good balance as most other projects also refer to it that way, although referring to them as "unused headers" is also reasonable.

varungandhi-apple · 2020-05-30T07:13:13Z

@swift-ci please test

varungandhi-apple · 2020-05-31T04:36:20Z

Following up here: I think there are one or more bugs in clang-format/git-clang-format. Here is the patch it computed for me:

diff --git a/include/swift/AST/ASTPrinter.h b/include/swift/AST/ASTPrinter.h
index 0e5a17c9dce..a1a04d8e21b 100644
--- a/include/swift/AST/ASTPrinter.h
+++ b/include/swift/AST/ASTPrinter.h
@@ -13,15 +13,15 @@
 #ifndef SWIFT_AST_ASTPRINTER_H
 #define SWIFT_AST_ASTPRINTER_H
 
+#include "swift/AST/Identifier.h"
+#include "swift/AST/PrintOptions.h"
 #include "swift/Basic/LLVM.h"
 #include "swift/Basic/QuotedString.h"
 #include "swift/Basic/UUID.h"
-#include "swift/AST/Identifier.h"
+#include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringRef.h"
-#include "llvm/ADT/DenseSet.h"
 #include "llvm/Support/raw_ostream.h"
-#include "swift/AST/PrintOptions.h"
 
 namespace swift {
   class Decl;
diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp
index 1633f20d3ee..2a5e5ca8edb 100644
--- a/lib/AST/Decl.cpp
+++ b/lib/AST/Decl.cpp
@@ -15,11 +15,11 @@
 //===----------------------------------------------------------------------===//
 
 #include "swift/AST/Decl.h"
-#include "swift/AST/AccessRequests.h"
-#include "swift/AST/AccessScope.h"
 #include "swift/AST/ASTContext.h"
-#include "swift/AST/ASTWalker.h"
 #include "swift/AST/ASTMangler.h"
+#include "swift/AST/ASTWalker.h"
+#include "swift/AST/AccessRequests.h"
+#include "swift/AST/AccessScope.h"
 #include "swift/AST/DiagnosticEngine.h"
 #include "swift/AST/DiagnosticsSema.h"
 #include "swift/AST/ExistentialLayout.h"
@@ -29,7 +29,6 @@
 #include "swift/AST/GenericSignature.h"
 #include "swift/AST/Initializer.h"
 #include "swift/AST/LazyResolver.h"
-#include "swift/AST/ASTMangler.h"
 #include "swift/AST/Module.h"
 #include "swift/AST/NameLookup.h"
 #include "swift/AST/NameLookupRequests.h"
@@ -41,9 +40,14 @@
 #include "swift/AST/ResilienceExpansion.h"
 #include "swift/AST/SourceFile.h"
 #include "swift/AST/Stmt.h"
+#include "swift/AST/SwiftNameTranslation.h"
 #include "swift/AST/TypeCheckRequests.h"
 #include "swift/AST/TypeLoc.h"
-#include "swift/AST/SwiftNameTranslation.h"
+#include "swift/Basic/Range.h"
+#include "swift/Basic/Statistic.h"
+#include "swift/Basic/StringExtras.h"
+#include "swift/Basic/TypeID.h"
+#include "swift/Demangling/ManglingMacros.h"
 #include "swift/Parse/Lexer.h"
 #include "clang/Lex/MacroInfo.h"
 #include "llvm/ADT/SmallPtrSet.h"
@@ -52,11 +56,6 @@
 #include "llvm/ADT/Statistic.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/raw_ostream.h"
-#include "swift/Basic/Range.h"
-#include "swift/Basic/StringExtras.h"
-#include "swift/Basic/Statistic.h"
-#include "swift/Basic/TypeID.h"
-#include "swift/Demangling/ManglingMacros.h"
 
 #include "clang/Basic/CharInfo.h"
 #include "clang/AST/Attr.h"
diff --git a/lib/AST/GenericSignatureBuilder.cpp b/lib/AST/GenericSignatureBuilder.cpp
index ee348386c10..11d54b88722 100644
--- a/lib/AST/GenericSignatureBuilder.cpp
+++ b/lib/AST/GenericSignatureBuilder.cpp
@@ -16,11 +16,11 @@
 //
 //===----------------------------------------------------------------------===//
 
-#include "GenericSignatureBuilderImpl.h"
 #include "swift/AST/GenericSignatureBuilder.h"
+#include "GenericSignatureBuilderImpl.h"
 #include "swift/AST/ASTContext.h"
-#include "swift/AST/DiagnosticsSema.h"
 #include "swift/AST/DiagnosticEngine.h"
+#include "swift/AST/DiagnosticsSema.h"
 #include "swift/AST/ExistentialLayout.h"
 #include "swift/AST/FileUnit.h"
 #include "swift/AST/GenericEnvironment.h"
@@ -36,17 +36,17 @@
 #include "swift/Basic/Debug.h"
 #include "swift/Basic/Defer.h"
 #include "swift/Basic/Statistic.h"
-#include "llvm/ADT/GraphTraits.h"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/GraphTraits.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/Statistic.h"
-#include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/GraphWriter.h"
-#include "llvm/Support/raw_ostream.h"
 #include "llvm/Support/SaveAndRestore.h"
+#include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 
 using namespace swift;
diff --git a/lib/Frontend/ModuleInterfaceSupport.cpp b/lib/Frontend/ModuleInterfaceSupport.cpp
index b22c456fb72..447ab766a06 100644
--- a/lib/Frontend/ModuleInterfaceSupport.cpp
+++ b/lib/Frontend/ModuleInterfaceSupport.cpp
@@ -10,6 +10,7 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "swift/Frontend/ModuleInterfaceSupport.h"
 #include "swift/AST/ASTContext.h"
 #include "swift/AST/ASTPrinter.h"
 #include "swift/AST/Decl.h"
@@ -21,7 +22,6 @@
 #include "swift/AST/ProtocolConformance.h"
 #include "swift/Basic/STLExtras.h"
 #include "swift/Frontend/Frontend.h"
-#include "swift/Frontend/ModuleInterfaceSupport.h"
 #include "swift/Frontend/PrintingDiagnosticConsumer.h"
 #include "swift/SILOptimizer/PassManager/Passes.h"
 #include "swift/Serialization/SerializationOptions.h"
diff --git a/lib/SILOptimizer/LoopTransforms/ArrayPropertyOpt.cpp b/lib/SILOptimizer/LoopTransforms/ArrayPropertyOpt.cpp
index 2f2adfe656b..3a1bc5db883 100644
--- a/lib/SILOptimizer/LoopTransforms/ArrayPropertyOpt.cpp
+++ b/lib/SILOptimizer/LoopTransforms/ArrayPropertyOpt.cpp
@@ -53,16 +53,16 @@
 #define DEBUG_TYPE "array-property-opt"
 
 #include "ArrayOpt.h"
-#include "swift/SILOptimizer/Analysis/LoopAnalysis.h"
-#include "swift/SILOptimizer/PassManager/Transforms.h"
-#include "swift/SILOptimizer/Utils/CFGOptUtils.h"
-#include "swift/SILOptimizer/Utils/SILSSAUpdater.h"
 #include "swift/SIL/CFG.h"
 #include "swift/SIL/DebugUtils.h"
 #include "swift/SIL/InstructionUtils.h"
-#include "swift/SIL/Projection.h"
 #include "swift/SIL/LoopInfo.h"
+#include "swift/SIL/Projection.h"
 #include "swift/SIL/SILCloner.h"
+#include "swift/SILOptimizer/Analysis/LoopAnalysis.h"
+#include "swift/SILOptimizer/PassManager/Transforms.h"
+#include "swift/SILOptimizer/Utils/CFGOptUtils.h"
+#include "swift/SILOptimizer/Utils/SILSSAUpdater.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
diff --git a/lib/SILOptimizer/Transforms/PhiArgumentOptimizations.cpp b/lib/SILOptimizer/Transforms/PhiArgumentOptimizations.cpp
index 817cdd5eab7..72f34f0779e 100644
--- a/lib/SILOptimizer/Transforms/PhiArgumentOptimizations.cpp
+++ b/lib/SILOptimizer/Transforms/PhiArgumentOptimizations.cpp
@@ -15,10 +15,10 @@
 //===----------------------------------------------------------------------===//
 
 #define DEBUG_TYPE "sil-optimize-block-arguments"
-#include "swift/SILOptimizer/PassManager/Transforms.h"
-#include "swift/SIL/SILBasicBlock.h"
 #include "swift/SIL/SILArgument.h"
+#include "swift/SIL/SILBasicBlock.h"
 #include "swift/SIL/SILFunction.h"
+#include "swift/SILOptimizer/PassManager/Transforms.h"
 #include "swift/SILOptimizer/Utils/CFGOptUtils.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/Support/Debug.h"
diff --git a/lib/Sema/ConstraintSystem.cpp b/lib/Sema/ConstraintSystem.cpp
index 81fd3d0e324..953e2091552 100644
--- a/lib/Sema/ConstraintSystem.cpp
+++ b/lib/Sema/ConstraintSystem.cpp
@@ -16,13 +16,13 @@
 //
 //===----------------------------------------------------------------------===//
 #include "ConstraintSystem.h"
-#include "ConstraintGraph.h"
 #include "CSDiagnostics.h"
 #include "CSFix.h"
+#include "ConstraintGraph.h"
 #include "SolutionResult.h"
 #include "TypeCheckType.h"
-#include "swift/AST/Initializer.h"
 #include "swift/AST/GenericEnvironment.h"
+#include "swift/AST/Initializer.h"
 #include "swift/AST/ParameterList.h"
 #include "swift/AST/TypeCheckRequests.h"
 #include "swift/Basic/Statistic.h"
diff --git a/lib/Serialization/ModuleFile.h b/lib/Serialization/ModuleFile.h
index 60e66e3fc4d..00fcaa9982a 100644
--- a/lib/Serialization/ModuleFile.h
+++ b/lib/Serialization/ModuleFile.h
@@ -14,15 +14,15 @@
 #define SWIFT_SERIALIZATION_MODULEFILE_H
 
 #include "ModuleFormat.h"
+#include "swift/AST/FileUnit.h"
 #include "swift/AST/Identifier.h"
 #include "swift/AST/LazyResolver.h"
 #include "swift/AST/LinkLibrary.h"
-#include "swift/AST/FileUnit.h"
 #include "swift/AST/Module.h"
 #include "swift/AST/RawComment.h"
 #include "swift/AST/TypeLoc.h"
-#include "swift/Serialization/Validation.h"
 #include "swift/Basic/LLVM.h"
+#include "swift/Serialization/Validation.h"
 #include "clang/AST/Type.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"

It is being overly aggressive in rearranging #includes. If we analyze the first example, only one commit is modifying ASTPrinter.h (9fa8f87); it is adding #include "llvm/ADT/SmallString.h" in what is arguably the correct place, before llvm/ADT/StringRef.h and after the swift section. However, clang-format is more aggressive, (I'm guessing) it sees that the other includes are not in alphabetical order and rearranging both the swift and llvm sections.

The other examples are similar.

As for include-what-you-use, I do have it finally working. Here's a gist of the output (I haven't fiddled with IWYU's settings, this output was produced with the default settings). Some suggestions seem janky, like this one:

/Users/varun/foss-swift/swift/stdlib/include/llvm/ADT/Hashing.h should add these lines:
#include <stdint.h>                      // for uint64_t, uint32_t, uint8_t
#include "cstdint"                       // for uint64_t

I don't want to use this PR to discuss this more, this is mostly FYI.

I'm hoping to submit a documentation PR on building/running/roadblocks some time next week and create a forum post to discuss potential steps forward. I'll cc you on the documentation PR and forum post.

varungandhi-apple · 2020-05-31T04:53:17Z

@swift-ci please test

varungandhi-apple · 2020-05-31T17:55:37Z

@swift-ci please test Windows platform

compnerd · 2020-05-31T19:39:57Z

I don't follow what you believe to be a bug in clang-format/git-clang-format. The changes apply to a bit of context and the lines that are changed. You should use clang-format with a specific range as otherwise it normally applies to the entire file.

I don't think that the suggestions there are janky - they are valid. The specific sized typedef require those headers. Can you share why you believe them to be janky?

Awesome, thank you for writing up the documentation for this, I think it will make it easier for others to replicate this.

compnerd · 2020-05-31T19:40:48Z

The windows failure seems legitimate:

FAILED: lib/IRGen/CMakeFiles/swiftIRGen.dir/IRGenRequests.cpp.obj 
C:\PROGRA~2\MICROS~1\2019\COMMUN~1\VC\Tools\MSVC\1424~1.283\bin\Hostx64\x64\cl.exe  /nologo /TP -DCMARK_STATIC_DEFINE -DGTEST_HAS_RTTI=0 -DLLVM_ON_WIN32 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_CRT_USE_WINAPI_FAMILY_DESKTOP_APP -D_DLL -D_ENABLE_ATOMIC_ALIGNMENT_FIX -D_ENABLE_EXTENDED_ALIGNED_STORAGE=1 -D_HAS_EXCEPTIONS=0 -D_HAS_STATIC_RTTI=0 -D_MD -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Ilib\IRGen -IS:\jenkins\workspace\swift-PR-windows\swift\lib\IRGen -Iinclude -IS:\jenkins\workspace\swift-PR-windows\swift\include -IS:\jenkins\workspace\swift-PR-windows\llvm\include -IT:\llvm\include -IS:\jenkins\workspace\swift-PR-windows\clang\include -IT:\llvm\tools\clang\include -IS:\jenkins\workspace\swift-PR-windows\cmark\src -IT:\cmark\src -IS:\jenkins\workspace\swift-PR-windows\swift-corelibs-libdispatch\src\BlocksRuntime -IS:\jenkins\workspace\swift-PR-windows\swift-corelibs-libdispatch -I"C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.24.28314\\include" -I"C:\Program Files (x86)\Windows Kits\10\\Include\10.0.18362.0\ucrt" -I"C:\Program Files (x86)\Windows Kits\10\\Include\10.0.18362.0\shared" -I"C:\Program Files (x86)\Windows Kits\10\\Include\10.0.18362.0\um" /GS- /Oy /Zc:inline /Zc:strictStrings /Oi /Zc:rvalueCast /W4 -wd4141 -wd4146 -wd4244 -wd4267 -wd4291 -wd4345 -wd4351 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4709 -wd4324 -w14062 -we4238 /we4062 /wd4068 /permissive- -DOBJC_OLD_DISPATCH_PROTOTYPES=0 /MD /O2 /Ob2   -UNDEBUG  /EHs-c- /GR- -O2 /GR- -U_DEBUG -UNDEBUG -std:c++14 /showIncludes /Folib\IRGen\CMakeFiles\swiftIRGen.dir\IRGenRequests.cpp.obj /Fdlib\IRGen\CMakeFiles\swiftIRGen.dir\swiftIRGen.pdb /FS -c S:\jenkins\workspace\swift-PR-windows\swift\lib\IRGen\IRGenRequests.cpp
S:\jenkins\workspace\swift-PR-windows\swift\include\swift/Basic/CTypeIDZone.def(39): error C2039: 'vector': is not a member of 'std'
C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.24.28314\\include\functional(28): note: see declaration of 'std'
S:\jenkins\workspace\swift-PR-windows\swift\include\swift/Basic/CTypeIDZone.def(39): error C2065: 'vector': undeclared identifier
S:\jenkins\workspace\swift-PR-windows\swift\include\swift/Basic/CTypeIDZone.def(39): error C2974: 'swift::TypeID': invalid template argument for 'T', type expected
S:\jenkins\workspace\swift-PR-windows\swift\include\swift/Basic/TypeID.h(55): note: see declaration of 'swift::TypeID'
S:\jenkins\workspace\swift-PR-windows\swift\include\swift/Basic/CTypeIDZone.def(39): error C2988: unrecognizable template declaration/definition
S:\jenkins\workspace\swift-PR-windows\swift\include\swift/Basic/CTypeIDZone.def(39): error C2143: syntax error: missing ';' before '>'
S:\jenkins\workspace\swift-PR-windows\swift\include\swift/Basic/CTypeIDZone.def(39): error C2059: syntax error: '>'
S:\jenkins\workspace\swift-PR-windows\swift\include\swift/Basic/CTypeIDZone.def(39): error C2143: syntax error: missing ';' before '{'
S:\jenkins\workspace\swift-PR-windows\swift\include\swift/Basic/CTypeIDZone.def(39): error C2447: '{': missing function header (old-style formal list?)
S:\jenkins\workspace\swift-PR-windows\swift\include\swift/Basic/CTypeIDZone.def(39): error C2988: unrecognizable template declaration/definition
S:\jenkins\workspace\swift-PR-windows\swift\include\swift/Basic/CTypeIDZone.def(39): error C2059: syntax error: '>'
S:\jenkins\workspace\swift-PR-windows\swift\include\swift/Basic/CTypeIDZone.def(39): error C2039: 'value': is not a member of '`global namespace''
S:\jenkins\workspace\swift-PR-windows\swift\include\swift/Basic/CTypeIDZone.def(40): error C2988: unrecognizable template declaration/definition
S:\jenkins\workspace\swift-PR-windows\swift\include\swift/Basic/CTypeIDZone.def(40): error C2143: syntax error: missing ';' before '<'
S:\jenkins\workspace\swift-PR-windows\swift\include\swift/Basic/CTypeIDZone.def(40): error C2374: 'swift::TypeID': redefinition; multiple initialization
S:\jenkins\workspace\swift-PR-windows\swift\include\swift/Basic/CTypeIDZone.def(39): note: see declaration of 'swift::TypeID'
S:\jenkins\workspace\swift-PR-windows\swift\include\swift/Basic/CTypeIDZone.def(40): error C2059: syntax error: '<'
S:\jenkins\workspace\swift-PR-windows\swift\include\swift/Basic/CTypeIDZone.def(40): error C2065: 'T': undeclared identifier
S:\jenkins\workspace\swift-PR-windows\swift\include\swift/Basic/CTypeIDZone.def(40): error C2923: 'std::unique_ptr': 'T' is not a valid template type argument for parameter '_Ty'

varungandhi-apple · 2020-05-31T20:22:02Z

I don't follow what you believe to be a bug in clang-format/git-clang-format. The changes apply to a bit of context and the lines that are changed. You should use clang-format with a specific range as otherwise it normally applies to the entire file.

Hmm. I thought if I used the diff tool, it would automatically narrow down the context. I'll retry. FWIW, the way I ran it was

(PATH="../build/Ninja-ReleaseAssert/llvm-macosx-x86_64/bin:$PATH"; ../llvm-project/clang/tools/clang-format/git-clang-format ea92df04e13e2a6dadc90c762b4a4b24a2aa2a0d)

git-clang-format --help says:

If zero or one commits are given, run clang-format on all lines that differ
between the working directory and <commit>, which defaults to HEAD.  Changes are
only applied to the working directory.

If two commits are given (requires --diff), run clang-format on all lines in the
second <commit> that differ from the first <commit>.

Are you running the command with specific flags to narrow down the diff further?

I don't think that the suggestions there are janky - they are valid. The specific sized typedef require those headers. Can you share why you believe them to be janky?

Maybe I'm missing something, but isn't <cstdint> sufficient? Why are both <stdint.h> and <cstdint> necessary?

The windows failure seems legitimate

Yeah, I re-added some includes which were mistakenly removed.

varungandhi-apple · 2020-05-31T21:02:04Z

@swift-ci please test Windows platform

compnerd · 2020-05-31T21:04:53Z

Are you running the command with specific flags to narrow down the diff further?

git-clang-format just invokes clang-format, but clang-format does apply changes to some context as well AFAIK. I normally run it explicitly on the line that Im editing, so it limits it to ~±2 lines.

Maybe I'm missing something, but isn't sufficient? Why are both <stdint.h> and necessary?

<stdint.h> provides the C types, <cstdint> provides the std:: namespace'd version. More importantly, the constants are split across the two (due to the __STDC_CONSTANT_MACROS that swift and LLVM build with). (I think that this is where we groan about C++?)

compnerd · 2020-05-31T21:05:40Z

@swift-ci please test Windows platform

compnerd · 2020-06-01T01:30:10Z

@swift-ci please clean test macOS platform

compnerd · 2020-06-01T01:30:36Z

@swift-ci please clean test Linux platform

swift-ci · 2020-06-01T03:36:00Z

Build failed
Swift Test Linux Platform
Git Sha - 77dbf62

varungandhi-apple · 2020-06-01T05:34:08Z

There seems to be absolutely no difference in compile times. 🤷

/usr/bin/time -p utils/build-script --skip-watchos --skip-tvos --skip-build-ios-simulator --skip-build-benchmark --release --swift-darwin-supported-archs 'x86_64'
* before:
  real 1627.33
  user 25485.47
  sys 1116.88
* after:
  real 1619.04
  user 25488.07
  sys 1108.77

cd ../build/Ninja-ReleaseAssert/swift-macosx-x86_64
find lib name '*.o' -type f -delete
/usr/bin/time -p ninja
* before:
  real 399.75
  user 5532.17
  sys 242.22
* after:
  real 399.67
  user 5530.14
  sys 241.34

cd ../build/Ninja-ReleaseAssert/swift-macosx-x86_64
find lib name '*.o' -type f -delete
/usr/bin/time -p ninja swift
* before:
  real 287.96
  user 5034.64
  sys 208.26
* after:
  real 287.54
  user 5027.38
  sys 207.93

compnerd · 2020-06-01T15:00:32Z

Aww, that’s too bad. Would be expected if you are building with modules, not without modules. This is still a clean up that’s good to do IMO, and we should see it through. Thanks for grabbing the numbers!

compnerd · 2020-06-01T15:01:00Z

@swift-ci please test Linux platform

varungandhi-apple requested a review from CodaFi May 29, 2020 10:42

CodaFi approved these changes May 29, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

varungandhi-apple force-pushed the vg-tidying-up-without-marie-kondo branch from cb6cbe8 to 8eb7fc3 Compare May 31, 2020 04:53

This comment has been minimized.

Sign in to view

varungandhi-apple added 3 commits May 31, 2020 13:05

[NFC] Remove redundant includes for <vector>.

ea92df0

[NFC] Remove redundant includes for <string>.

4028222

[NFC] Remove redundant includes for llvm/ADT/Optional.h.

2c7b4b1

varungandhi-apple added 16 commits May 31, 2020 13:06

[NFC] Remove redundant includes for llvm/ADT/StringRef.h.

3077cbd

[NFC] Remove redundant includes for llvm/ADT/ArrayRef.h.

0441894

[NFC] Remove redundant includes for llvm/ADT/TinyPtrVector.h.

ba4b43a

[NFC] Remove redundant includes for llvm/ADT/SmallVector.h.

96a332e

[NFC] Remove redundant includes for llvm/ADT/SmallString.h.

52dcd47

[NFC] Remove redundant includes for llvm/ADT/SmallSet.h.

c14e934

[NFC] Remove redundant includes for llvm/ADT/SmallPtrSet.h.

91693e9

[NFC] Remove redundant includes for llvm/ADT/StringSet.h.

81ce068

[NFC] Remove redundant includes for llvm/ADT/StringMap.h.

e64d4a0

[NFC] Remove redundant includes for llvm/ADT/DenseSet.h.

fea589e

[NFC] Remove redundant includes for llvm/ADT/DenseMap.h.

7df7378

[NFC] Remove redundant includes for llvm/ADT/SetVector.h.

f854921

[NFC] Remove redundant includes for llvm/ADT/MapVector.h.

caf38b6

[NFC] Remove redundant includes for llvm/ADT/BitVector.h.

5e73d77

[NFC] Remove self-include in PassPipeline.h.

82e87f6

[NFC] IWYU for swift/Basic/TypeID.h.

77dbf62

varungandhi-apple force-pushed the vg-tidying-up-without-marie-kondo branch from 8eb7fc3 to 77dbf62 Compare May 31, 2020 20:15

varungandhi-apple merged commit 2419112 into swiftlang:master Jun 2, 2020

varungandhi-apple deleted the vg-tidying-up-without-marie-kondo branch June 2, 2020 02:51

varungandhi-apple mentioned this pull request Jun 2, 2020

[docs] Add a how-to guide on running IWYU on the Swift project. #32151

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get rid of #includes that do not spark joy #32080

Get rid of #includes that do not spark joy #32080

varungandhi-apple commented May 29, 2020

varungandhi-apple commented May 29, 2020

swift-ci commented May 29, 2020

varungandhi-apple commented May 29, 2020

compnerd commented May 29, 2020

varungandhi-apple commented May 29, 2020

compnerd commented May 29, 2020

varungandhi-apple commented May 29, 2020

varungandhi-apple commented May 29, 2020 •

edited

Loading

compnerd commented May 29, 2020

varungandhi-apple commented May 29, 2020

swift-ci commented May 29, 2020

compnerd commented May 29, 2020

varungandhi-apple commented May 29, 2020 •

edited

Loading

compnerd commented May 29, 2020

varungandhi-apple commented May 30, 2020

This comment has been minimized.

This comment has been minimized.

varungandhi-apple commented May 31, 2020

varungandhi-apple commented May 31, 2020

This comment has been minimized.

This comment has been minimized.

varungandhi-apple commented May 31, 2020

compnerd commented May 31, 2020

compnerd commented May 31, 2020

varungandhi-apple commented May 31, 2020 •

edited

Loading

varungandhi-apple commented May 31, 2020

compnerd commented May 31, 2020

compnerd commented May 31, 2020

compnerd commented Jun 1, 2020

compnerd commented Jun 1, 2020

swift-ci commented Jun 1, 2020

varungandhi-apple commented Jun 1, 2020

compnerd commented Jun 1, 2020

compnerd commented Jun 1, 2020

Get rid of #includes that do not spark joy #32080

Get rid of #includes that do not spark joy #32080

Conversation

varungandhi-apple commented May 29, 2020

varungandhi-apple commented May 29, 2020

swift-ci commented May 29, 2020

varungandhi-apple commented May 29, 2020

compnerd commented May 29, 2020

varungandhi-apple commented May 29, 2020

compnerd commented May 29, 2020

varungandhi-apple commented May 29, 2020

varungandhi-apple commented May 29, 2020 • edited Loading

compnerd commented May 29, 2020

varungandhi-apple commented May 29, 2020

swift-ci commented May 29, 2020

compnerd commented May 29, 2020

varungandhi-apple commented May 29, 2020 • edited Loading

compnerd commented May 29, 2020

varungandhi-apple commented May 30, 2020

This comment has been minimized.

This comment has been minimized.

varungandhi-apple commented May 31, 2020

varungandhi-apple commented May 31, 2020

This comment has been minimized.

This comment has been minimized.

varungandhi-apple commented May 31, 2020

compnerd commented May 31, 2020

compnerd commented May 31, 2020

varungandhi-apple commented May 31, 2020 • edited Loading

varungandhi-apple commented May 31, 2020

compnerd commented May 31, 2020

compnerd commented May 31, 2020

compnerd commented Jun 1, 2020

compnerd commented Jun 1, 2020

swift-ci commented Jun 1, 2020

varungandhi-apple commented Jun 1, 2020

compnerd commented Jun 1, 2020

compnerd commented Jun 1, 2020

varungandhi-apple commented May 29, 2020 •

edited

Loading

varungandhi-apple commented May 29, 2020 •

edited

Loading

varungandhi-apple commented May 31, 2020 •

edited

Loading