Noissue - Avoid redundant lookup in keyword-by-filter map

lib/matcher.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 328 matching lines @@
   /**
    * Checks whether a particular filter is slow
    * @param {RegExpFilter} filter
    * @returns {boolean}
    */
   isSlowFilter(filter)
   {
     let matcher = (
       filter instanceof WhitelistFilter ? this.whitelist : this.blacklist
     );
-    if (matcher.hasFilter(filter))
-      return !matcher.getKeywordForFilter(filter);
+    let keyword = matcher.getKeywordForFilter(filter);
+    if (keyword != null)
+      return !keyword;

[hub, 2018/09/13 15:52:40]

Why the `!keyword`. I'd just return the proper boolean value here.

[Manish Jethani, 2018/09/14 05:28:32]

Do you mean `keyword != ""`?

[hub, 2018/09/14 12:20:49]

no. just `return true`

[Manish Jethani, 2018/09/14 13:36:25]

Well it should be false if the keyword is an empty string.

[Jon Sonesen, 2018/09/17 19:13:18]

So, returning !keyword here would return true in the case where keyword is an
empty string, and false if it is not empty. Is that correct? perhaps based
solely on the discussion here it makes sense to return a bool for readability? I
do see that that would add an additional step. Personally, I find that this
approach makes sense and fits the previous convention (although that is not an
argument one way or another just a fact)

[Manish Jethani, 2018/09/17 23:36:35]

Sorry, I got it backwards. It should return true if it's empty. So it should be
`return !keyword` or `return keyword == ""`.
We can't return either `true` or `false` ... unless you're suggesting `return
keyword == "" ? true : false` (which doesn't seem more readable to me, YMMV).

[hub, 2018/09/18 12:19:40]

I see. Works for me as it is.

     return !matcher.findKeyword(filter);
   }
 
   /**
    * Optimized filter matching testing both whitelist and blacklist matchers
    * simultaneously. For parameters see
      {@link Matcher#matchesAny Matcher.matchesAny()}.
    * @see Matcher#matchesAny
    * @inheritdoc
    */
@@ skipping 52 matching lines @@
 
 exports.CombinedMatcher = CombinedMatcher;
 
 /**
  * Shared {@link CombinedMatcher} instance that should usually be used.
  * @type {CombinedMatcher}
  */
 let defaultMatcher = new CombinedMatcher();
 
 exports.defaultMatcher = defaultMatcher;