Include detection refactor #174
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This function never fails, so no need to return and check for an error value. Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>
Originally, this code was written to find all include statements in a source file, for further processing. However, since a while the include finding was changed to run the preprocessor to see if any includes are missing, and this class is used to find the missing includes in the error message. In practice, the preprocessor will bail out at the first missing include, since there is no meaningful way for it to continue. This means there will be at most one include in the error output. If there would be multiple includes (either because the preprocessor decided to continue, or there is some other output that accidentially matches the regex), it would be harmful to actually process all of them, since the absence of the first include might influence the presence of the second (e.g. in the presence of #ifdefs). This commit enforces that only one include is returned at a time, slightly simplifying the code in the process. The filename and class should be renamed to singular as well, but this will be left for a future commit to reduce noise. Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>
Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>
Previously, during include detection, every pass through IncludesToIncludeFolders would iterate over all included filenames found so far, and rebuild the list of include folders from the list of included libraries again. This commit consistes of these changes: - IncludesToIncludeFolders now looks at ctx.IncludeJustFound to find new libraries (instead of ctx.Includes which contains all included filenames found so far). To minimize changes in this commit, it still creates a slice with just the single include found and processes that. A future commit will simplify this. - With that change, ctx.Includes is no longer used so it is removed. - The resolveIncludeFolders function is removed. It used to build the list of include folders, based on the list of imported libaries. This list was built from scratch again every time, including the core and variant folders. These two folders are now added once, by ContainerFindIncludes, and adding the library's source folders to the include folder list is done by IncludesToIncludeFolders. - ContainerFindIncludes would start by calling IncludesToIncludeFolders (even before any includes were detected) to ensure that the include path contained the core and variant folders on the first preprocessor run already. Now that these paths are added by ContainerFindIncludes, there is no need for this extra call to IncludesToIncludeFolders, so it is removed. - resolveLibraries is modified to actually return only newly imported libraries, not all imported libraries. - resolveLibrary now returns the library it resolved to, instead of just adding it to the markImportedLibrary map. Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>
This code was written to handle processing of multiple includes after each other, but it only needs to handle one. This allows simplifying the code a bit. Behaviour should not be changed. Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>
The removed code iterates over the detected libraries and adds their source directories to the queue for running further detection on them. However, IncludesToIncludeFolders (called by findIncludesUntilDone(), which is called a bit above the removed code) already does this, so there is no need to do it again. Since Ctx.FoldersWithSources is a UniqueStringQueue, this unneeded code was not affecting behaviour in any way. It seems likely that this code had a purpose previously, but it was lost in some refactor. However, a quick scan of the commit that introduced it (d6e378: Implemented library to library dependency full discovery) suggests that this code was useless from the start. Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>
Previously, it was called once for the main cpp file, and then in a loop for any extra source files from the sketch or dependent libraries. However, this can be simplified by just adding the main .cpp file to the queue, and then letting the loop handle it. This produces a tiny change in the order in which source files are processed for includes, but any setups affected by this are already prone to errors. Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>
This field was used during include detection, to collect the source folders whose source files should be processed for includes. This field was handled by the CollectAllSourceFilesFromFoldersWithSources pass, which finds all source files in these folders and adds them to the Context.CollectedSourceFiles queue instead. This commit changes all code that used to add directories to the FoldersWithSourceFiles queue to instead add all contained source files to the CollectedSourceFiles queue. For this, the CollectAllSourceFilesFromFoldersWithSources pass is transformed into regular function and renamed to QueueSourceFilesInFolder, so it can be directly called without needing any particular context entries. Note that the filename that contains the latter function is no longer entirely accurate, but this will be fixed in a later commit (left untouched in this commit to simplify review). Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>
This prepares for a change in the next commit. Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>
In previous commits, this method has been signficantly simplified, reducing it to little more than a wrapper around the resolveLibrary helper function. This commit renames resolveLibrary to ResolveLibrary to make it public and lets findIncludesUntilDone call it directly (instead of going through IncludesToIncludeFolders). findIncludesUntilDone now also takes care of processing the ResolveLibrary result, which was previously done by IncludesToIncludeFolders. The signature of ResolveLibrary is also changed to accept a Context, so it can now extract the needed variables itself, instead of needing IncludesToIncludeFolders to extract them. Note that the filename that contains ResolveLibrary is no longer entirely accurate, but this will be fixed in a later commit (to simplify review of this commit). Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>
This function used to be a pass, but it was reduced to a regular function in 1001916 (Remove Context.FoldersWithSourceFiles). By now, the function is only called from container_find_includes.go, so it makes sense to just move it into that file. This does not change any code, it just moves the function and renames it to be private. Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>
Since 631b09f (Remove IncludesToIncludeFolders.Run), the primary function in this file changed to become ResolveLibrary. This updates the filename to match. No code is changed, this just renames a file. Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>
|
✅ Build completed. ⬇️ Build URL: ℹ️ To test this build:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR does a lot of refactoring, cleanup and simplification of the include detection code. It should not affect behaviour in any significant way, since it mostly changes structure of the existing code. In particular, the
ResolveLibraryfunction that takes care of the actual include -> library resolution is left untouched, since it contains code for some very specific corner cases (even though it also really needs a refactor, removing some useless parts of the code).This PR is made on top of #172, so the first couple of commits should be ignored until that one is merged.