#4 Update TDEL10n module - add excludes

Merged
SlavekB merged 1 commits from feat/CMakeL10n-excludes into master 10 months ago

As mentioned in TDE/tdelibs#8, it would be useful to have a way to determine EXCLUDES. The patch contained here adds this option.

As mentioned in TDE/tdelibs#8, it would be useful to have a way to determine EXCLUDES. The patch contained here adds this option.
MicheleC reviewed 10 months ago
need to review exclude modification loop
@@ -271,0 +283,4 @@
foreach( _src ${_files} )
foreach( _exclude ${_excludes} )
if( ${_src} MATCHES ${_exclude} )
list( REMOVE_ITEM _files ${_src} )
MicheleC

I am not sure how cmake handles that, but usually modifying a list that is also being iterated with a foreach at the same time, is a mistake. See line 283 and 285.

You should either

  1. make a copy of the list to iterate, then iterate over the copy and modify the original
  2. or iterate the original list, make a list of items to remove, then iterate the second list and remove items from the original list
  3. or iterate the original list backward from back to front. In this case it is usually safe to modify the list as you iterate
I am not sure how cmake handles that, but usually modifying a list that is also being iterated with a foreach at the same time, is a mistake. See line 283 and 285. You should either 1. make a copy of the list to iterate, then iterate over the copy and modify the original 2. or iterate the original list, make a list of items to remove, then iterate the second list and remove items from the original list 3. or iterate the original list backward from back to front. In this case it is usually safe to modify the list as you iterate
SlavekB

I have already tested this procedure several times and it works successfully. I think CMake will evaluate items for the foreach at the beginning of the cycle and then the changes in the list will have no undue impact on the cycle.

I have already tested this procedure several times and it works successfully. I think CMake will evaluate items for the foreach at the beginning of the cycle and then the changes in the list will have no undue impact on the cycle.
SlavekB

By the way, the same procedure is used also in loops for pick resource files (line 305) and desktop files (line 353).

By the way, the same procedure is used also in loops for pick resource files (line 305) and desktop files (line 353).
MicheleC

cmake documentation does not add anything that helps. it may work fine, but still it is not best practice because we are relying on cmake to save/copy the list initially and then iterate over all items. Should this behavior change in future versions of cmake, we would have new bugs to fix.

If you believe it is safe enough due to previous testing, then ok.

cmake documentation does not add anything that helps. it may work fine, but still it is not best practice because we are relying on cmake to save/copy the list initially and then iterate over all items. Should this behavior change in future versions of cmake, we would have new bugs to fix. If you believe it is safe enough due to previous testing, then ok.
MicheleC

you could do something like this (not sure about the exact cmake syntax):

copy listA to listAA
foreach over listAA
  remove item from listA
empty listAA
you could do something like this (not sure about the exact cmake syntax): ``` copy listA to listAA foreach over listAA remove item from listA empty listAA ```
SlavekB

I researched it again. Passing ${_files} to foreach(…) means, that the value of _files list is expanded to its current content and passed as unquoted argument – see Unquoted Argument and Variable References in CMake documentation.

Because the value of list is expanded into arguments before the foreach(…) starts then such use is and will be safe.

I researched it again. Passing `${_files}` to `foreach(…)` means, that the value of `_files` list is expanded to its current content and passed as unquoted argument – see [Unquoted Argument](https://cmake.org/cmake/help/v3.0/manual/cmake-language.7.html#unquoted-argument) and [Variable References](https://cmake.org/cmake/help/v3.0/manual/cmake-language.7.html#variable-references) in CMake documentation. Because the value of list is expanded into arguments before the `foreach(…)` starts then such use is and will be safe.
SlavekB

In any case, I have now found the solution that will be the best: list(FILTER …)

This could serve for all the cases we have now used. Because this is a change that applies not only to this PR but also to the existing code, I will probably prepare a separate patch => another commit and PR in this series.

In any case, I have now found the solution that will be the best: `list(FILTER …)` This could serve for all the cases we have now used. Because this is a change that applies not only to this PR but also to the existing code, I will probably prepare a separate patch => another commit and PR in this series.
MicheleC

Great, list (FILTER… ) looks good and it is even simpler to code :smile:

Great, list (FILTER... ) looks good and it is even simpler to code :smile:
SlavekB

Bad news: The list( FILTER … ) is new in CMake 3.6, while we hold CMake 2.8 support. So I leave the existing code with foreach( … ) as it is now.

Bad news: The `list( FILTER … )` is new in CMake 3.6, while we hold CMake 2.8 support. So I leave the existing code with `foreach( … )` as it is now.
MicheleC

ok. suggest we add a comment to make clear the modifying foreach look is safe :smile:

ok. suggest we add a comment to make clear the modifying foreach look is safe :smile:
SlavekB deleted branch feat/CMakeL10n-excludes 10 months ago
SlavekB added this to the R14.0.6 release milestone 10 months ago
The pull request has been merged.
Sign in to join this conversation.
Loading…
Cancel
Save
There is no content yet.