Implements #215 #221

wmjordan · 2018-03-02T01:04:55Z

This implements #215 by allowing users to annotate members with RepackDropAttribute (customizable via /repackdrop:AttributeName).

…ing." This reverts commit 29a3793.

…le via "repackdrop")

timotei · 2018-03-02T07:58:34Z

ILRepack/RepackImporter.cs

@@ -119,7 +119,11 @@ public MethodReference Import(MethodReference reference, IGenericParameterProvid

        public TypeDefinition Import(TypeDefinition type, Collection<TypeDefinition> col, bool internalize)
        {
-            _logger.Verbose("- Importing " + type);
+			if (CanInclude(type) == false) {


Current code uses spaces (4 spaces) instead of tab, let's keep consistency

I prefer tabs in all my projects.
Can you upload an editor config file to ensure that spacing consistency (as well as others) so I can follow it?

timotei · 2018-03-02T07:59:14Z

ILRepack/RepackImporter.cs

@@ -119,7 +119,11 @@ public MethodReference Import(MethodReference reference, IGenericParameterProvid

        public TypeDefinition Import(TypeDefinition type, Collection<TypeDefinition> col, bool internalize)
        {
-            _logger.Verbose("- Importing " + type);
+			if (CanInclude(type) == false) {
+				_logger.Log("Repack dropped " + type.FullName);


This is also logged in the CanInclude method, I'd say we only log once here, and rewrite it as; Type <X> dropped as it was marked with the drop attribute

You are right. I forgot that CanInclude also logged that. Shall I simply remove the log here?

timotei · 2018-03-02T08:00:23Z

ILRepack/RepackImporter.cs

-        /// Clones a field to a newly created type
-        /// </summary>
-        private void CloneTo(FieldDefinition field, TypeDefinition nt)
+		private bool CanInclude<TMember>(TMember member) where TMember : ICustomAttributeProvider, IMemberDefinition {


Could be renamed to ShouldDropForRepacking?

OK, I renamed that method and use an if in each place to filter the result where CloneTo was called

timotei · 2018-03-02T08:01:59Z

ILRepack/RepackOptions.cs

-            InputAssemblies = cmd.OtherAguments;
+			RepackDropAttribute = cmd.Option("repackdrop");
+			if (String.IsNullOrWhiteSpace(RepackDropAttribute)) {
+				RepackDropAttribute = "RepackDropAttribute";


This will always drop the types flagged with the attribute, I think we want to only do this if the option is enabled. (/repackdrop or /repackdrop:attribute)

OK, I agree with that.

…option. Performance tweak for the RepackDrop support.

wmjordan · 2018-03-05T01:03:09Z

Could anyone tell me why the AppVeyor build failed?

timotei · 2018-03-05T08:24:19Z

@wmjordan Yes, it seems that the tests failed:

8139  Not run: 0, Invalid: 0, Ignored: 0, Skipped: 0
8140
8141Errors and Failures:
81421) Test Failure : ILRepack.IntegrationTests.Scenarios.GivenDotNet462AppReferencingMicrosoftBclAsyncAndSystemRuntime_MergedApplicationRunsSuccessfully
8143     Process has not ended.
8144  Expected: True
8145  But was:  False
8146
8147at ILRepack.IntegrationTests.Scenarios.RunScenario(String scenarioName) in C:\projects\il-repack\ILRepack.IntegrationTests\Scenarios.cs:line 69```

wmjordan · 2018-03-05T08:54:05Z

Thank you for replying and spotting it out.
I did not have DotNet 4.6.2 SDK on my VS. I unloaded the project. Could it be the reason which leaded to the failure of the test?

gluck

Some minor remarks but LGTM overall.
Would have to check by the test fails though, but I fear it might be unrelated.

gluck · 2018-03-08T09:27:20Z

ILRepack/RepackImporter.cs

@@ -120,6 +120,11 @@ public MethodReference Import(MethodReference reference, IGenericParameterProvid
        public TypeDefinition Import(TypeDefinition type, Collection<TypeDefinition> col, bool internalize)
        {
            _logger.Verbose("- Importing " + type);
+            bool hasFilter = String.IsNullOrEmpty(_options.RepackDropAttribute) == false;
+            if (hasFilter && ShouldDrop(type))


considering moving hasFilter check inside ShouldDrop

I thought placing the result (hasFilter) outside of ShouldDrop can save some calculations since it would not change during merging.

For performance considerations, maybe it is better to promote it to a read-only field.
For code clearance, placing it into ShouldDrop might be reasonable.

What kind of approach would you prefer?

Or shall I refactor both hasFilter and ShouldDrop into a new type, for instance RepackFilter?

Appreciate the concern for performance, but I prefer code clarity over performance speculation (IMO perf difference won't be noticeable, partly because inlining and partly because it's negligible compared to the rest of the processing here)

OK, moved hasFilter check into ShouldDrop

gluck · 2018-03-08T09:27:33Z

ILRepack/RepackOptions.cs

@@ -97,6 +97,8 @@ public List<string> AllowedDuplicateNameSpaces
            get { return allowedDuplicateNameSpaces; }
        }

+		public string RepackDropAttribute { get; set; }


indent is off by 1 here

Yep, you are right. That file needs reformatting.

gluck · 2018-03-08T09:27:42Z

ILRepack/RepackOptions.cs

@@ -224,8 +226,15 @@ void Parse()
            LogVerbose = cmd.Modifier("verbose");
            LineIndexation = cmd.Modifier("index");

-            // everything that doesn't start with a '/' must be a file to merge (verify when loading the files)
-            InputAssemblies = cmd.OtherAguments;
+			if (cmd.HasOption("repackdrop")) {


here as well

wmjordan · 2018-03-10T01:05:29Z

The new commit managed to pass the checks.
The test failure might not be related to this pull request.

wmjordan added 4 commits March 1, 2018 18:21

Allow to use RepackDropAttribute to exclude members from merging.

29a3793

Revert "Allow to use RepackDropAttribute to exclude members from merg…

fb91908

…ing." This reverts commit 29a3793.

Support dropping members or types via RepackDropAttribute (customizab…

7ad7a25

…le via "repackdrop")

Merge branch 'master' of https://github.com/wmjordan/il-repack

9276c88

timotei reviewed Mar 2, 2018

View reviewed changes

wmjordan added 2 commits March 3, 2018 18:40

RepackDrop support should be enabled explicitly with the /repackdrop …

279afd3

…option. Performance tweak for the RepackDrop support.

Recommit /repackdrop option support

168e48e

gluck reviewed Mar 8, 2018

View reviewed changes

Refactored /repackdrop support for code clarity

e9ba83f

gluck merged commit a02073e into gluck:master Mar 12, 2018

wmjordan mentioned this pull request Mar 13, 2018

Feature request: conditionally merge duplicated members #215

Closed

wmjordan mentioned this pull request Feb 25, 2020

[Feature] Strip a type mkaring/ConfuserEx#53

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implements #215 #221

Implements #215 #221

wmjordan commented Mar 2, 2018

timotei Mar 2, 2018

wmjordan Mar 2, 2018

timotei Mar 2, 2018

wmjordan Mar 3, 2018

timotei Mar 2, 2018

wmjordan Mar 3, 2018

timotei Mar 2, 2018

wmjordan Mar 2, 2018

wmjordan commented Mar 5, 2018

timotei commented Mar 5, 2018

wmjordan commented Mar 5, 2018

gluck left a comment

gluck Mar 8, 2018

wmjordan Mar 8, 2018 •

edited

Loading

wmjordan Mar 8, 2018

gluck Mar 8, 2018

wmjordan Mar 9, 2018

gluck Mar 8, 2018

wmjordan Mar 8, 2018

gluck Mar 8, 2018

wmjordan commented Mar 10, 2018 •

edited

Loading

Implements #215 #221

Implements #215 #221

Conversation

wmjordan commented Mar 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wmjordan commented Mar 5, 2018

timotei commented Mar 5, 2018

wmjordan commented Mar 5, 2018

gluck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wmjordan Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wmjordan commented Mar 10, 2018 • edited Loading

wmjordan Mar 8, 2018 •

edited

Loading

wmjordan commented Mar 10, 2018 •

edited

Loading